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

New: Add suggestions API #12384

Merged
merged 13 commits into from Nov 22, 2019
Merged

New: Add suggestions API #12384

merged 13 commits into from Nov 22, 2019

Conversation

@wdoug
Copy link
Contributor

wdoug commented Oct 7, 2019

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)

  1. 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.
  2. 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
Copy link

jsf-clabot commented Oct 7, 2019

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Oct 7, 2019
@wdoug wdoug mentioned this pull request Oct 7, 2019
@ilyavolodin
Copy link
Member

ilyavolodin commented Oct 7, 2019

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

@ilyavolodin ilyavolodin added accepted core feature and removed triage labels Oct 9, 2019
Copy link
Member

ilyavolodin left a comment

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 Outdated Show resolved Hide resolved
docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
Copy link
Member

platinumazure left a comment

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
Copy link
Member

ilyavolodin commented Oct 10, 2019

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

@wdoug
Copy link
Contributor Author

wdoug commented Oct 23, 2019

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 wdoug force-pushed the wdoug:suggestions branch from 4bc32da to 228518a Nov 11, 2019
@wdoug wdoug force-pushed the wdoug:suggestions branch from 228518a to 6f904f0 Nov 11, 2019
data: { character },
suggest: [
{
messageId: "removeEscape",

This comment has been minimized.

Copy link
@wdoug

wdoug Nov 11, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@platinumazure

platinumazure Nov 12, 2019

Member

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.

This comment has been minimized.

Copy link
@ilyavolodin

ilyavolodin Nov 12, 2019

Member

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

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

This comment has been minimized.

Copy link
@wdoug

wdoug Nov 11, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@ilyavolodin

ilyavolodin Nov 13, 2019

Member

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.

This comment has been minimized.

Copy link
@platinumazure

platinumazure Nov 13, 2019

Member

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.

This comment has been minimized.

Copy link
@wdoug

wdoug Nov 14, 2019

Author Contributor

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
Copy link
Contributor Author

wdoug commented Nov 12, 2019

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.

@wdoug wdoug requested review from platinumazure and ilyavolodin Nov 12, 2019
@wdoug
Copy link
Contributor Author

wdoug commented Nov 21, 2019

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
Copy link
Member

ilyavolodin commented Nov 22, 2019

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!

Copy link
Member

btmills left a comment

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

Copy link
Member

kaicataldo left a comment

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

}
},
{
messageId: "escapeBackslash",

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Nov 22, 2019

Member

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

@kaicataldo kaicataldo merged commit 6eaad96 into eslint:master Nov 22, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191121.2 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@wdoug wdoug deleted the wdoug:suggestions branch Nov 22, 2019
@wdoug
Copy link
Contributor Author

wdoug commented Nov 22, 2019

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
Copy link
Member

kaicataldo commented Nov 22, 2019

This should get in today's release!

@eslint eslint bot locked and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.