Skip to content

Simplify type names should fix all diagnostics with the same ID.#26823

Merged
jinujoseph merged 5 commits intodotnet:masterfrom
CyrusNajmabadi:simplifyNameFixAll
Jun 7, 2018
Merged

Simplify type names should fix all diagnostics with the same ID.#26823
jinujoseph merged 5 commits intodotnet:masterfrom
CyrusNajmabadi:simplifyNameFixAll

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented May 12, 2018

Fixes: #26532

Current fix-all for simplify type names only fixes if the ID is the same and the contents of the node are the same. This is not really that helpful, and means when a person sees a huge swath of "simplify name to ..." suggestions in their file ("IDE0001"), they can't actually fix tehm all at one. They need to fix-all over and over again for all the different types they have.

This is basically unlike any of our fix-alls which almost always try to sensibly do their work for all the diagnostics of that same ID.

When reviewed, i recommend going a commit at a time.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 12, 2018 22:06
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Tagging @dotnet/roslyn-ide . Not this is a followup PR to #26822

Tagging @sharwell This is a change in behavior. However, i contend the previous behavior was broken and not at all in line with how the rest of our fix-all features work.

@CyrusNajmabadi CyrusNajmabadi changed the title Simplify type names should fix all dia gnostics with teh same ID. Simplify type names should fix all diagnostics with the same ID. May 12, 2018
{
int i1 = 0;
System.Int16 s1 = 0;
short s1 = 0;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this behavior is strictly better. The user the "prefer intrinsic types" option set. They then ask to "fix all" the suggestion to use predefined types in tihs file. But we would only fix up one type of type. I can't imagine this being more desirable than the user actually getting all cases fixed up where they can use a predefined type given that that's their optoin and they said 'fix all'.

@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented May 12, 2018

This is currently blocked on the design review in #26532. The discussion of the proposal along with the design discussion result will be added to that issue, and this pull request can be considered if/when the new design is accepted.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@sharwell Sounds good. Do you think you can get this to a design meeting soon?

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Need Design Review The end user experience design needs to be reviewed and approved. labels May 13, 2018
@sharwell
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi It didn't fit into our time in the last one but we're trying to get through the design backlog as quickly as possible.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@sharwell Can you guys potentially take a look next time? Thanks!

@sharwell sharwell removed Blocked Need Design Review The end user experience design needs to be reviewed and approved. labels Jun 5, 2018
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@sharwell @jinujoseph please merge in when convenient. Thanks!

@jinujoseph jinujoseph merged commit cdf70ed into dotnet:master Jun 7, 2018
@davkean
Copy link
Copy Markdown
Member

davkean commented Nov 9, 2019

@CyrusNajmabadi This was a while ago, but thank-you again for making this change - so much better!

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

you're welcome @davkean. Glad it's better!

@CyrusNajmabadi CyrusNajmabadi deleted the simplifyNameFixAll branch November 9, 2019 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Name can be simplified" fixer is not helpful

5 participants