Skip to content

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented May 2, 2022

Follow up to #60480 and #60557

@mavasani mavasani requested review from CyrusNajmabadi and jmarolf May 2, 2022 06:02
@mavasani mavasani requested a review from a team as a code owner May 2, 2022 06:02
@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2022

@Youssef1313 for review

{
// always offer to convert from verbatim string to normal string.
context.RegisterRefactoring(new MyCodeAction(
context.RegisterRefactoring(CodeAction.CreateWithPriority(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new internal helper CodeAction.CreateWithPriority for creating code actions with non-default priority. We no longer need to create special code action sub-types just to override the priority.

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Few missing code actions that were not named "MyCodeAction"

  • PreferFrameworkTypeCodeFixProvider (PreferFrameworkTypeCodeAction)
  • AbstractConvertAutoPropertyToFullPropertyCodeRefactoringProvider (ConvertAutoPropertyToFullPropertyCodeAction)
  • AbstractConvertForEachToLinqQueryProvider (ForEachToLinqQueryCodeAction)
  • AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider (ConvertToInterpolatedStringCodeAction)
  • AbstractConvertForEachToForCodeRefactoringProvider (ForEachToForCodeAction)

Other than that, LGTM 🎉

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I'm confused regarding the RegisterCodeFix(context, ... introduced in #60665 vs context.RegisterCodeFix(...).

Which should be used?

@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2022

I'm confused regarding the RegisterCodeFix(context, ... introduced in #60665 vs context.RegisterCodeFix(...).

Which should be used?

I believe Tomas just introduced a helper in that PR, so if the fund you were passing to RegisterCodeFix only invokes the base type’s FixAsync method, then you should use the new helper to reduce verbosity.

@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2022

Thanks @Youssef1313, that helped! I actually found a few more redundant sub-types, all of them should be removed with the latest commit.

Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here! Do we have tests for code action priority that ensure we are laying things out in the right order?

@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2022

Thanks for all the work here! Do we have tests for code action priority that ensure we are laying things out in the right order?

I think we have for some, not for all. Let me create an issue to track adding the tests.

@mavasani
Copy link
Contributor Author

mavasani commented May 2, 2022

Filed #61081

@mavasani mavasani merged commit 01b0022 into dotnet:main May 2, 2022
@mavasani mavasani deleted the RemoveMyCodeAction branch May 2, 2022 14:40
@ghost ghost added this to the Next milestone May 2, 2022
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants