-
Notifications
You must be signed in to change notification settings - Fork 172
Description
I might be in a minority for that, but i think dotnet format should not populate missing switch branches with throw new NotImplementedException(). Scenario:
I had this code in codebase:
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
_ => query.OrderBy(e => e.CreatedAt)
};Even though there is TasksSortBy.Unspecified - this code is just fine, since we have the wildcard _ in the place to handle it. But i still want to keep the diagnostic to avoid missing enum values that are "real". So the diagnostic is fine, maybe Unspecified is too much (but it's a good practise from gRPC protocol, so we are using it there). My issue is with the automatic fix. After running the dotnet format i ended up with:
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
TasksSortBy.Unspecified => throw new NotImplementedExcetion(),
_ => query.OrderBy(e => e.CreatedAt)
};which for me it is outright dangerous. Fortunately for us - the integration test suite quickly caught the issue in CI pipeline, but the developer did not check the code after running the dotnet format (which is understandable, it should be just format). In this case format command changed the code behaviour. I'm opening this thread to figure out if more people think like me here :).
To be clear - the "solution" isn't that clear either, but from my perspective it would be better if dotnet format left the code "uncompilable" at this point. So:
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
TasksSortBy.Unspecified => // TODO : implement that
_ => query.OrderBy(e => e.CreatedAt)
};is much better, since it's forcing the developer to implement the missing branch. What do you all think about that?