Suggests parameter name for IntroduceLocal on an argument #10072
Conversation
| } | ||
| } | ||
|
|
||
| public static ArgumentInfo GetArgumentInfo(this SemanticModel semanticModel, ArgumentSyntax argument) |
There was a problem hiding this comment.
probably shouldn't be public. Private?
There was a problem hiding this comment.
Oh, I see. I didn't realize this was the SemanticModelExtensions class. This is fine to have public here.
There was a problem hiding this comment.
I'm pretty certain there is an IOperation call that gives you this information directly. Ask @JohnHamby if that's true.
There was a problem hiding this comment.
That'd be cool. Though this is definitely a missing SemanticModel API.
There was a problem hiding this comment.
I can use IOperation but I'm waiting for #10082.
|
Would it be interesting to test parameters from things other than methods? For example, indexers, or delegates? Does it work with property setters or is inferring "value" unhelpful? Does it work with overloaded operators (e.g. if I extract the LHS of a user-defined addition operator)? |
| } | ||
| else | ||
| { | ||
| var argumentList = parent.Parent as ArgumentListSyntax; |
There was a problem hiding this comment.
What about BracketedArgumentList?
|
Also, the intent of my bug was that we would pick this name as the last thing we do if no other matches happen. i.e. if you write: Then the variable introduces should be called "Customer", not "Item/Value/Whatever". i.e. only if we were going to fallback to our "can't think of a good name" codepath would we do these extra checks. |
| { | ||
| if (parent.NameColon != null) | ||
| { | ||
| return parent.NameColon.Name.Identifier.ValueText.ToCamelCase(); |
There was a problem hiding this comment.
- Do we care if the name is missing?
- Do we care if the name is wrong (i.e. squiggled)?
- Why do we need to call ToCamelCase? Shouldn't we follow the casing of the named argument?
|
Do we need to test the case where the parameter name is not a valid identifier in the calling language? |
|
ETA test failures are unrelated |
|
@dotnet-bot retest prtest/win/dbg/eta please |
|
Does/should this work for the receiver of a reduced extension method? |
|
IMO, using hte parameter name should only be the last resort if any other options didn't work. That's what i intended when i filed the bug. We already have good rules for what to do based on the expression itself. This is about what we do when none of those current rules work, and we need to use some additional context to help determine the name. |
|
@CyrusNajmabadi I disagree that a type name is in general better than a parameter name :) |
|
We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them. If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you! |
sharwell
added
the
Resolution-Expired
Fixes #2423.
@dotnet/roslyn-ide for review 🎉