Respect PreferParameterNullChecking user option #59561
Conversation
ghost
added
the
Community
Youssef1313
force-pushed
the
respect-paramnull-option-59559
branch
from
5e98baf
to
b7be789
Compare
| [InlineData("", true)] | ||
| [InlineData("csharp_style_prefer_parameter_null_checking = true", true)] | ||
| [InlineData("csharp_style_prefer_parameter_null_checking = false", false)] | ||
| public async Task TestSimpleReferenceType(string editorConfig, bool useParameterNullChecking) |
There was a problem hiding this comment.
💡 Consider rewriting this to minimize unnecessary/duplicate parameterization of the method:
| [InlineData("", true)] | |
| [InlineData("csharp_style_prefer_parameter_null_checking = true", true)] | |
| [InlineData("csharp_style_prefer_parameter_null_checking = false", false)] | |
| public async Task TestSimpleReferenceType(string editorConfig, bool useParameterNullChecking) | |
| [InlineData(null)] | |
| [InlineData(true)] | |
| [InlineData(false)] | |
| public async Task TestSimpleReferenceType(bool? useParameterNullChecking) |
There was a problem hiding this comment.
My preference would be to extract the scenario where the option is explicitly disabled to its own [Fact], and then treat this test as though useParameterNullChecking is a constant true. However, I don't feel strongly about any particular factoring of the test.
There was a problem hiding this comment.
@sharwell @RikkiGibson I'm a bit confused as to which suggestion should I take. Both sounds reasonable and I'm okay either way.
There was a problem hiding this comment.
Let's go with #59561 (comment) :)
There was a problem hiding this comment.
The downside of the approach mentioned by @RikkiGibson is we lose the ability to see that the two tests are operating on different input. At that point, it's not clear whether the different output is caused by a change in the option, or a change in the input. While this is rarely a problem in the pull request where the test is introduced, it's easy and common for an update to one test to fail to realize that the other is a matching test that needs to be updated at the same time. For this reason, it is strongly recommended for the test to be combined.
There was a problem hiding this comment.
@Youssef1313 I won't block this PR on the suggested change. More a note for the future to keep with the approach you were already using 😄
| [InlineData("", true)] | ||
| [InlineData("csharp_style_prefer_parameter_null_checking = true", true)] | ||
| [InlineData("csharp_style_prefer_parameter_null_checking = false", false)] | ||
| public async Task TestSimpleReferenceType(string editorConfig, bool useParameterNullChecking) |
There was a problem hiding this comment.
My preference would be to extract the scenario where the option is explicitly disabled to its own [Fact], and then treat this test as though useParameterNullChecking is a constant true. However, I don't feel strongly about any particular factoring of the test.
|
@Youssef1313 Please advise us if you plan to make any change to the test, and otherwise I think we can just merge. |
| [InlineData("csharp_style_prefer_parameter_null_checking = false", false)] | ||
| public async Task TestSimpleReferenceType(string editorConfig, bool useParameterNullChecking) | ||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)] | ||
| public async Task TestSimpleReferenceType() |
There was a problem hiding this comment.
It looks like the case where the option is explicitly enabled was removed. Was that intended?
|
📝 CLA bot seems to be down. Have reported this and will ping that check after it's fixed/online again to trigger the merge. |
Fixes #59559
Relevant Discord discussion that lead to this PR: