Handle patterns in populate switch expressions#50984
Handle patterns in populate switch expressions#50984CyrusNajmabadi merged 8 commits intodotnet:mainfrom
Conversation
src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
Outdated
Show resolved
Hide resolved
…essionHelpers.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
b7ab60a to
c39956f
Compare
src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
Show resolved
Hide resolved
src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
Outdated
Show resolved
Hide resolved
| (MyEnum)1 => 2, | ||
| ""Mismatching constant"" => 3, | ||
| _ => throw new System.NotImplementedException(), | ||
| MyEnum.FizzBuzz => throw new System.NotImplementedException(), |
There was a problem hiding this comment.
When "Mismatching constant" was encountered, the analyzer considered the switch as "complete". Hence, it failed to 'add missing cases'.
The "mismatching constant" is a compile-error anyway. So it probably doesn't matter a lot.
| Bar.Option2 => 2, | ||
| null => null, | ||
| _ => throw new System.NotImplementedException(), | ||
| Bar.Option3 => throw new System.NotImplementedException(), |
There was a problem hiding this comment.
This test is misleading. It's named TestAddMissingCasesForNullableEnum, but the enum in it isn't nullable. Do you want me to make the enum nullable in it?
There was a problem hiding this comment.
dont' change existing tests. you can rename them and/or add new tests. thanks!
There was a problem hiding this comment.
@CyrusNajmabadi Well, I'll leave the test case and test name as-is for now. Only the expected result is updated.
There was a problem hiding this comment.
wfm. feel free to update the name as well if you want. but genrally don't make the test fit the name, make the name fit the test. and we feel there is a test hole, where no test checks the original name's case, then we can add that :)
|
Want me to merge? or are you still working on things here? |
|
@CyrusNajmabadi You can merge. Let me know if you want me to file an issue for that? |
No issue needed. We generally are not good about effectively managing such issues. So the repo just fills up without end. |
|
@CyrusNajmabadi Anything left to merge? |
|
Thanks! |
Fixes #50982
Not sure of the fix quality tbh.