Strip common prefixes before applying naming suggestions#26589
Strip common prefixes before applying naming suggestions#26589sharwell merged 6 commits intodotnet:masterfrom
Conversation
…vider.cs The new name matches the contained type name.
|
I think I saw some separate tests specifically for generating and updating names somewhere, and I think they were in VB. So that might be a better place to put all these cases in. |
|
@Neme12 I wasn't able to find any naming styles tests for VB. Do you know where they are? |
|
Sorry, I wasn't clear enough. The tests don't test VB code, they're just written in VB. I found them: But given that they only test the internal helpers, not the code fix itself to make sure everything is wired up properly, your addition is still useful as is. |
| [InlineData("M__", "_")] | ||
| [InlineData("S_", "_s_")] | ||
| [InlineData("T_", "_t_")] | ||
| [InlineData("M_S__T_", "_t_")] |
There was a problem hiding this comment.
expected tests with just _'s or also, _m also tests of something like _moop
| prefix: "", | ||
| suffix: "", | ||
| wordSeparator: ""); | ||
| var namingRule = new SerializableNamingRule() |
There was a problem hiding this comment.
nit: either add blank lines between all of the statements (preferred) or remove the one above.
| prefix: "_", | ||
| suffix: "", | ||
| wordSeparator: ""); | ||
| var namingRule = new SerializableNamingRule() |
There was a problem hiding this comment.
nit: either add blank lines between all of the statements (preferred) or remove the one above.
| var index = 0; | ||
| while (name.Length > index + 1) | ||
| { | ||
| switch (char.ToLowerInvariant(name[index])) |
There was a problem hiding this comment.
nit: I don't think the case indentation below matches the repo style.
| private static string StripCommonPrefixes(string name) | ||
| { | ||
| var index = 0; | ||
| while (name.Length > index + 1) |
There was a problem hiding this comment.
nit: I kind of expected to see this expression inverted as I think it reads better. Totally not a big deal though. That's kind of in the eye of the beholder.
There was a problem hiding this comment.
Oh, I assumed from the form that name was getting written in the loop. Since it's not written, I definitely expected to see this the other way around.
| { | ||
| case 'm': | ||
| case 's': | ||
| case 't': |
There was a problem hiding this comment.
For now I just used the prefixes deemed "common" by the old StyleCop implementation. We can add others if we want to, but probably as a separate issue. Want me to file a bug for this?
| [InlineData("M__", "_")] | ||
| [InlineData("S_", "s_")] | ||
| [InlineData("T_", "t_")] | ||
| [InlineData("M_S__T_", "t_")] |
There was a problem hiding this comment.
What tests that start with underscores but have a prefix, like __s_bar? The algorithm covers those too.
There was a problem hiding this comment.
The algorithm covers these, but none of the tests will work currently due to #26588. I expect to see the remaining test cases added as part of that work, but I can also add placeholders (skipped) here if you want.
|
Approved to merge for 15.8.Preview4 |
Fixes #26566
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.