Skip to content

Add 'fixAllDescription' property to CodeFixAction#22616

Merged
7 commits merged into
masterfrom
fixAllDescription
Mar 28, 2018
Merged

Add 'fixAllDescription' property to CodeFixAction#22616
7 commits merged into
masterfrom
fixAllDescription

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 15, 2018

Fixes #21380

@ghost ghost requested review from DanielRosenwasser and amcasey March 15, 2018 22:42
@ghost ghost force-pushed the fixAllDescription branch from 7408893 to ae036dc Compare March 16, 2018 17:28
@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 27, 2018

@DanielRosenwasser @amcasey Could someone review?

Copy link
Copy Markdown
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

No objections but not reviewed in fine detail.

Comment thread src/compiler/diagnosticMessages.json Outdated
"category": "Message",
"code": 95020
},
"Fix all like: ": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't love this working. @DanielRosenwasser ?

Comment thread src/compiler/diagnosticMessages.json Outdated
"category": "Message",
"code": 95024
},
"Prefix all unused declarations with '_'": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this only apply to parameters?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's determined by function canPrefix in fixUnusedIdentifier.ts -- so for the most part just parameters. I'll add where possible to the text.

Comment thread src/compiler/diagnosticMessages.json Outdated
"category": "Message",
"code": 95025
},
"Fix all spelling errors": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might want to hedge here because we are definitely not going to fix all spelling errors. 😄

Copy link
Copy Markdown
Author

@ghost ghost Mar 27, 2018

Choose a reason for hiding this comment

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

Changing to "fix all detected spelling errors"

Comment thread src/services/types.ts Outdated
* This may be omitted to indicate that the code fix can't be applied in a group.
*/
fixId?: {};
fixAllDescription?: SymbolDisplayPart[];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

VS is not expected to consume this, correct?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Don't see why you couldn't -- what's the current behavior to display a fix-all?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We show a separate dialog with a list of possible scopes:
https://www.visualstudio.com/wp-content/uploads/2016/11/Productivity-Epic-960x540.jpg

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, guess you won't need this then.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this not just string?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thought we might like the ability to highlight things (not used in this PR), but I can just use string if that's not needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we might like the ability to highlight things

do you have an example in mind?

but I can just use string if that's not needed.

if we have a specific use case, then it is fine, otherwise, i would not add it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Can't think of an example.


verify.codeFixAll({
fixId: "annotateWithTypeFromJSDoc",
fixAllDescription: "Fix all like: Annotate with type from JSDoc",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dedicated string?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More generally, why not customize every description?

@ghost ghost force-pushed the fixAllDescription branch from e67ddb9 to 1c9f836 Compare March 27, 2018 18:27
@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 27, 2018

CC @mjbvz for change to protocol.ts.

@ghost ghost force-pushed the fixAllDescription branch from 506f1ca to 0781c35 Compare March 27, 2018 18:49
@ghost ghost force-pushed the fixAllDescription branch from 007e85b to 3819e13 Compare March 27, 2018 19:53
Copy link
Copy Markdown
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Protocol changes lgtm

@ghost ghost force-pushed the fixAllDescription branch from 3819e13 to 4962c58 Compare March 27, 2018 20:27
@ghost ghost force-pushed the fixAllDescription branch from 4962c58 to 10c4881 Compare March 27, 2018 20:44
@ghost ghost merged commit 3e32e15 into master Mar 28, 2018
@ghost ghost deleted the fixAllDescription branch March 28, 2018 01:21
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants