-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle patterns in populate switch expressions #50984
Handle patterns in populate switch expressions #50984
Conversation
src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
Outdated
Show resolved
Hide resolved
…essionHelpers.cs Co-authored-by: CyrusNajmabadi <[email protected]>
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
@@ -962,7 +962,7 @@ void Method() | |||
(MyEnum)0 => 1, | |||
(MyEnum)1 => 2, | |||
""Mismatching constant"" => 3, | |||
_ => throw new System.NotImplementedException(), | |||
MyEnum.FizzBuzz => throw new System.NotImplementedException(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont' change existing tests. you can rename them and/or add new tests. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.