Skip to content
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

Fix bulk code fixes #299

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Fix bulk code fixes #299

merged 1 commit into from
Jun 19, 2018

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Jun 19, 2018

Removes or consolidates equivalentKey values so that batch fixers actually fix every diagnostic from a given analyzer where a particular resolution is available.

Fixes #277

@@ -59,7 +59,7 @@ public class VSTHRD002UseJtfRunCodeFixWithAwait : CodeFixProvider

return document.Project.Solution;
},
VSTHRD002UseJtfRunAnalyzer.Id),
"only action"),

Choose a reason for hiding this comment

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

we usually use title.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems reasonable. But as it doesn't appear to show up to the user anywhere, I think a static string should work too.

Choose a reason for hiding this comment

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

yep. it works. I am just saying title since title usually make debugging a bit easier since I don't need to dig but know right away where this fix is for most of time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. That does sound potentially useful. Where would I be debugging and see this value (presumably amidst other instances with different values)?

Copy link
Member Author

Choose a reason for hiding this comment

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

...and wouldn't the CodeAction.Title also be available to me in that context and be just as easy to read?

Copy link

@mavasani mavasani Jun 19, 2018

Choose a reason for hiding this comment

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

This string just defines an equivalence class of code actions that fix same diagnostic IDs in an identical fashion for a given fixer, and hence can be batched with FixAll. We recommend title as it generally provides clarity for the equivalence class, but any unique key will work.

Choose a reason for hiding this comment

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

not thing wrong with using "only action". for me, I usually just look equivalent key when debugging fix all. might be only because I am used to it. and yes, you can get title from codeaction if you have codeaction in hand.

@@ -75,7 +75,7 @@ internal VoidToTaskCodeAction(Document document, Diagnostic diagnostic)
public override string Title => Strings.VSTHRD100_CodeFix_Title;

/// <inheritdoc />
public override string EquivalenceKey => VSTHRD100AsyncVoidMethodAnalyzer.Descriptor.Id;
public override string EquivalenceKey => null;

Choose a reason for hiding this comment

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

so you don't want fix all for this fix?

Copy link
Member Author

@AArnott AArnott Jun 19, 2018

Choose a reason for hiding this comment

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

Sure I do. And it works. @mavasani's doc says:

Equivalence Key: A string value representing an equivalence class of all code actions registered by a code fixer or refactoring. Two code actions are treated as equivalent if they have equal non-null EquivalenceKey values and were generated by the same code fixer or refactoring.

But it's inaccurate. At least, it reads to me that a null equivalence key means it won't be batched. But I just tested it, and it works fine.

Choose a reason for hiding this comment

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

@mavasani seems a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Fixing" that bug (if that's what it is) would break existing code fix providers that intend to be batched and use null, FWIW.

Choose a reason for hiding this comment

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

This is by design. The default behavior for CodeAction is to have null equivalence key, and this makes the common user scenario of registering a single code fix and not providing an explicit equivalence key to work fine. The analyzer diagnostic is intended to advice providing an explicit non-null equivalence key for clarity and maintenance, so if in future you extend your fixer and register another code action, then you don't regress the FixAll for prior registered action.

@AArnott AArnott merged commit d9ff98f into microsoft:v15.8 Jun 19, 2018
@AArnott AArnott deleted the fix277 branch June 19, 2018 20:41
AArnott pushed a commit that referenced this pull request Oct 30, 2024
Bumps [Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning) from 3.6.143 to 3.6.146.
- [Release notes](https://github.com/dotnet/Nerdbank.GitVersioning/releases)
- [Commits](https://github.com/dotnet/Nerdbank.GitVersioning/commits)

---
updated-dependencies:
- dependency-name: Nerdbank.GitVersioning
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants