New: Add suggestions API #12384

wdoug · 2019-10-07T04:18:16Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR implements the proposal for a new suggestions API described in this rfc. This API will allow other tools, such as editors, to provide fixes for lint errors that, for various reasons, shouldn't be included as autofixes.
Updates the no-useless-escape rule to include the suggestions described here

Is there anything you'd like reviewers to focus on?

As a new contributor implementing a core addition, everything should probably be scrutinized closely. In particular, I would like some advice on tests, including additional tests that should be written.

jsf-clabot · 2019-10-07T04:18:17Z

All committers have signed the CLA.

docs/developer-guide/working-with-rules.md

docs/developer-guide/nodejs-api.md

docs/developer-guide/working-with-rules.md

docs/developer-guide/nodejs-api.md

ilyavolodin · 2019-10-07T12:58:41Z

@wdoug Thanks for working on this! I will review this a little later. cc @gaearon

ilyavolodin

I think we need to add some tests to verify that suggestions will not be included in formatter output.

docs/developer-guide/working-with-rules.md

docs/developer-guide/nodejs-api.md

platinumazure

Just to throw in an early wrinkle: Should we consider supporting something similar to messageId strings for suggestion descriptions, in order to allow for i18n/l10n of the suggestion descriptions?

I probably should have thought of this in the RFC stage, sorry.

ilyavolodin · 2019-10-10T19:31:37Z

@platinumazure Oh, good point. Yes, I think we should

wdoug · 2019-10-23T22:19:10Z

Just a heads up for anyone that is interested in this work. I am currently traveling internationally and it looks like I won't have time to work on this while I'm gone. If anyone is motivated to move this forward, go for it. Otherwise, I will try to get back to it when I'm available again in about 3 weeks.

wdoug · 2019-11-11T23:47:38Z

docs/developer-guide/working-with-rules.md

+            data: { character },
+            suggest: [
+                {
+                    messageId: "removeEscape",


I've implemented suggestions support for messageIds. Do we want to do it this way, or should it be a different key and different format in the meta info? For example, I could easily change this to be descriptionId and put the associated descriptions in meta.suggestionDescriptions or something instead of overloading meta.messages and messageIds. Thoughts?

Great question!

I don't have a problem with reusing meta.messages itself, but I would be curious to know if other team members think it could be confusing.

I see meta.messages as general-purpose bucket for i18n. So I think it should be fine to just reuse it.

wdoug · 2019-11-11T23:57:11Z

tests/lib/rules/no-useless-escape.js

+                type: "Literal",
+                suggestions: [{
+                    messageId: "removeEscape",
+                    output: "var foo = /;/;"


I pushed up this work-in-progress to get some feedback on the approach here. One option for these tests is to do something like what I have in the above test, where we assert against the suggestion fix data e.g.:

fix: { range: [11, 12], text: "" }

One problem with this is that it is hard to look at and verify that it will work properly.

An alternative approach that I was exploring was to use something like this output key where we define the result of applying the suggestion on the original code. This seems a lot easier to write good tests with, but it doesn't actually exist on the suggestion data so it could potentially be confusing. Thoughts?

Hmm.. That's a good question. I do think output approach is easier to deal with, but we don't have any way of getting that data (unless we run suggestions through auto-fix code inside tester)... So I think we'll have to stick with the ranges at least for now. We can change it later, if we find better approach.

I'd really like to make sure the user experience is good here-- otherwise, I'm worried nobody would use the feature. So I'm in favor of getting the output to work (and having RuleTester run the code through the autofixer as needed). It seems like this shouldn't cause regressions because it would only happen when someone adds a suggestion assertion object to the test case.

Let me know if I'm missing something.

Alright, I've added support for both options. See the Testing Suggestions section in docs/developer-guide/nodejs-api.md for docs.

I'm running the SourceCodeFixer.applyFixes when a suggestion output is specified, which helped me catch a bug in my original implementation of suggestions for the no-useless-escape rule which I definitely would have missed without this.

wdoug · 2019-11-12T00:02:59Z

Just getting around to working on this again. I pushed up some work-in-progress commits to get some feedback on the approach.

@platinumazure in response to your comment about "supporting something similar to messageId strings for suggestion descriptions, in order to allow for i18n/l10n of the suggestion descriptions", I have updated the code to directly support messageIds. I would love to get your feedback on this approach.

platinumazure · 2019-11-20T23:08:56Z

Sorry for the delay, I've been very busy and haven't had a lot of time for open source.

I'm kind of concerned about the tests. Why wouldn't we want to use text-based output and just run the autofixer against the suggestion's fix object? It would be much easier for users to test and it would be consistent with how autofix is tested outside of suggestions.

If the implementation has switched recently to use a text-base approach, that's great (and please see my comment about not having time for open source 😅). Otherwise, I guess I would like to understand what benefit we get from requiring users to know the structure of the fix object (and effectively making it a public API) to write tests for suggestions.

EDIT: I see that this implementation supports text output verification. Hurray! I see that it also supports checking the fix object itself. As I stated above, I'm mildly opposed to this because it effectively adds the fix object into the public API, and I don't think it really adds any value for users when most users should be used to output-based validation.

I appreciate your hard work on this and your willingness to respond to feedback, and I am grateful for your patience as I haven't been as available as I would like.

wdoug · 2019-11-21T05:31:50Z

tests/lib/rules/no-useless-escape.js

+                type: "Literal",
+                suggestions: [{
+                    messageId: "removeEscape",
+                    output: "var foo = /#/;"


I'm kind of concerned about the tests. Why wouldn't we want to use text-based output and just run the autofixer against the suggestion's fix object? It would be much easier for users to test and it would be consistent with how autofix is tested outside of suggestions.

If the implementation has switched recently to use a text-base approach, that's great (and please see my comment about not having time for open source 😅). Otherwise, I guess I would like to understand what benefit we get from requiring users to know the structure of the fix object (and effectively making it a public API) to write tests for suggestions.

@platinumazure, I have set up support for testing the output of what applying the suggestion would be and that is what I'm doing in these tests (as seen above). As noted here, this PR also currently supports testing the shape of the suggestion fix object. If you think we should only provide the output-based testing option, and not have the ability for users to test the fix object I'm fine with removing that option.

Thanks for commenting even though you have limited time. Let me know if there is anything else I can try to clarify.

Oh, I just saw your edited comment above. That seems reasonable. I will go ahead and remove the option to test the shape of the fix object.

Thanks so much! (I wasn't sure whether to edit or leave a second comment.)

Hoping to have time to do a full review this weekend.

wdoug · 2019-11-21T05:54:49Z

I see that it also supports checking the fix object itself. As I stated above, I'm mildly opposed to this because it effectively adds the fix object into the public API, and I don't think it really adds any value for users when most users should be used to output-based validation.

Alright, I've removed support for testing the suggestion fix object. If someone makes a valid pitch for why this would be valuable in the future it will be simple to re-add support.

Good idea to avoid features that we are not sure about. It is much easier to add them later than add them now and then later try to remove them after folks have started using those features.

ilyavolodin · 2019-11-22T01:30:05Z

From my perspective, this PR is in good shape. Would love for some other members of the @eslint/eslint-team to review it as well, but I think it's good enough. If no new reviews will be available in a few days, I will go ahead and merge it. @wdoug thank you for awesome job introducing this feature!

btmills

There was a lot to this, but it looks good to me - thanks @wdoug!

kaicataldo

LGTM. Excited to see how our integrations utilize this new feature. Thank you for implementing this!

kaicataldo · 2019-11-22T17:45:08Z

lib/rules/no-useless-escape.js

+                        }
+                    },
+                    {
+                        messageId: "escapeBackslash",


One thought going forward - do we want to create a convention that indicates which messages are for suggestions?

wdoug · 2019-11-22T17:55:06Z

Thanks for helping me work through this @ilyavolodin and @platinumazure, and thanks y'all for the detailed RFC and docs that helped guide me. One other question: Is there an expectation on when this might end up going out in a release?

kaicataldo · 2019-11-22T17:58:28Z

This should get in today's release!

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 7, 2019

wdoug commented Oct 7, 2019

View reviewed changes

docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved

wdoug commented Oct 7, 2019

View reviewed changes

docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved

wdoug commented Oct 7, 2019

View reviewed changes

docs/developer-guide/working-with-rules.md Show resolved Hide resolved

wdoug commented Oct 7, 2019

View reviewed changes

docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved

wdoug mentioned this pull request Oct 7, 2019

New: Suggestions eslint/rfcs#30

Merged

ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint and removed triage An ESLint team member will look at this issue soon labels Oct 9, 2019

ilyavolodin suggested changes Oct 9, 2019

View reviewed changes

docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved

docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved

platinumazure reviewed Oct 9, 2019

View reviewed changes

wdoug added 3 commits November 11, 2019 10:58

New: Initial suggestions support in the linter

ede4b0f

Docs: Update some docs with info about suggestions

b2e6419

Chore: Add some tests for suggestions

3ab339a

wdoug force-pushed the suggestions branch from 4bc32da to 228518a Compare November 11, 2019 22:20

wdoug added 4 commits November 11, 2019 15:24

New: Add support for suggestion messageId-based descriptions

aa99818

New: WIP add suggestions for no-useless-escape

d24afff

Chore: Update no-useless-escape suggestions to use messageIds

36d2b3b

New: Support output option for rule-tester tests of suggestions

6f904f0

wdoug force-pushed the suggestions branch from 228518a to 6f904f0 Compare November 11, 2019 23:39

wdoug commented Nov 11, 2019

View reviewed changes

wdoug requested review from platinumazure and ilyavolodin November 12, 2019 00:03

wdoug commented Nov 21, 2019

View reviewed changes

Chore: Remove rule-tester support for testing fix object

a08a135

btmills approved these changes Nov 22, 2019

View reviewed changes

kaicataldo approved these changes Nov 22, 2019

View reviewed changes

kaicataldo merged commit 6eaad96 into eslint:master Nov 22, 2019

wdoug deleted the suggestions branch November 22, 2019 17:51

wdoug mentioned this pull request Nov 22, 2019

Support new eslint suggestions API microsoft/vscode-eslint#806

Closed

snyk-bot mentioned this pull request Dec 1, 2019

[Snyk] Upgrade eslint from 6.6.0 to 6.7.0 basics/vector#111

Merged

This was referenced Dec 3, 2019

Support new eslint suggestions API SublimeLinter/SublimeLinter-eslint#289

Open

Support new eslint suggestions API AtomLinter/linter-eslint#1309

Open

snyk-bot mentioned this pull request Dec 11, 2019

[Snyk] Upgrade eslint from 6.7.0 to 6.7.2 basics/vector#113

Merged

snyk-bot mentioned this pull request Feb 13, 2020

[Snyk] Upgrade eslint from 6.5.1 to 6.8.0 thinkwee/thinkwee.github.io#39

Merged

snyk-bot mentioned this pull request Feb 27, 2020

[Snyk] Upgrade eslint from 5.15.1 to 6.8.0 alxtford/numconv#46

Merged

a-martynovich mentioned this pull request Mar 10, 2020

[Snyk] Upgrade eslint from 6.0.0 to 6.8.0 WoTTsecurity/api#773

Merged

snyk-bot mentioned this pull request Mar 11, 2020

[Snyk] Upgrade eslint from 6.0.0 to 6.8.0 WealthWizardsEngineering/instance-terminator#10

Merged

billyjanitsch mentioned this pull request Mar 14, 2020

relax react-hooks plugin peer dep? airbnb/javascript#2195

Closed

snyk-bot mentioned this pull request Mar 18, 2020

[Snyk] Upgrade eslint from 6.3.0 to 6.8.0 O330oei/node#4

Open

This was referenced Apr 3, 2020

[Snyk] Upgrade eslint from 5.4.0 to 6.8.0 LuisAlejandro/luisalejandro.github.io-legacy#79

Merged

[Snyk] Upgrade eslint from 6.2.0 to 6.8.0 CollageLabs/collagelabs.org#58

Merged

eslint-deprecated bot locked and limited conversation to collaborators May 22, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Add suggestions API #12384

New: Add suggestions API #12384

wdoug commented Oct 7, 2019 •

edited

jsf-clabot commented Oct 7, 2019 •

edited

ilyavolodin commented Oct 7, 2019

ilyavolodin left a comment

platinumazure left a comment

ilyavolodin commented Oct 10, 2019

wdoug commented Oct 23, 2019

wdoug Nov 11, 2019

platinumazure Nov 12, 2019

ilyavolodin Nov 12, 2019

wdoug Nov 11, 2019

ilyavolodin Nov 13, 2019

platinumazure Nov 13, 2019

wdoug Nov 14, 2019

wdoug commented Nov 12, 2019

platinumazure commented Nov 20, 2019 •

edited

wdoug Nov 21, 2019

wdoug Nov 21, 2019

platinumazure Nov 21, 2019

wdoug commented Nov 21, 2019 •

edited

ilyavolodin commented Nov 22, 2019

btmills left a comment

kaicataldo left a comment

kaicataldo Nov 22, 2019

wdoug commented Nov 22, 2019

kaicataldo commented Nov 22, 2019

New: Add suggestions API #12384

New: Add suggestions API #12384

Conversation

wdoug commented Oct 7, 2019 • edited

jsf-clabot commented Oct 7, 2019 • edited

ilyavolodin commented Oct 7, 2019

ilyavolodin left a comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

ilyavolodin commented Oct 10, 2019

wdoug commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdoug commented Nov 12, 2019

platinumazure commented Nov 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdoug commented Nov 21, 2019 • edited

ilyavolodin commented Nov 22, 2019

btmills left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdoug commented Nov 22, 2019

kaicataldo commented Nov 22, 2019

wdoug commented Oct 7, 2019 •

edited

jsf-clabot commented Oct 7, 2019 •

edited

platinumazure commented Nov 20, 2019 •

edited

wdoug commented Nov 21, 2019 •

edited