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

feat: directive prologue detection and autofix condition in quotes #17284

Merged
merged 3 commits into from Jun 16, 2023

Conversation

fasttime
Copy link
Member

Prerequisites checklist

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

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

see #17022

What changes did you make? (Give an overview)

This PR implements two changes in the quotes rule:

  • Fixed detection of directive prologues to account for parentheses around string literals, e.g. differentiate "use strict"; from ("use strict");. This can cause the rule to report new problems in certain cases.
  • Changed the logic that determines whether an autofix can be applied or not, in line with no-extra-parens: unparenthesized string literals that are direct children of an ExpressionStatement will not be autofixed.

Both changes were discussed in #17022, but we decided to postpone them to a new PR.

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

Shall we add a note to the documentation, like in no-extra-parens?

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jun 14, 2023
@fasttime fasttime added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly autofix This change is related to ESLint's autofixing capabilities and removed feature This change adds a new feature to ESLint labels Jun 14, 2023
@fasttime fasttime marked this pull request as ready for review June 14, 2023 06:04
@fasttime fasttime requested a review from a team as a code owner June 14, 2023 06:04
@mdjermanovic
Copy link
Member

Shall we add a note to the documentation, like in no-extra-parens?

I personally think there's no need to document edge cases where rules don't autofix the code, but we could add some "correct" examples for this rule with the "backtick" option where string literals must be string literals (e.g., directives and property names in object literals).

@nzakas
Copy link
Member

nzakas commented Jun 14, 2023

I think it's helpful to add a note to the documentation because there isn't any mention of directives right now. Something along the lines of, "This rule is aware of directive prologues such as "use strict" and will not flag or autofix them unless doing so will not change how the directive prologue is interpreted."

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jun 14, 2023
@fasttime
Copy link
Member Author

Thanks for your feedback @mdjermanovic @nzakas. I've updated the documentation as suggested (in 6f7b899).


Many codebases require strings to be defined in a consistent manner.

## Rule Details

This rule enforces the consistent use of either backticks, double, or single quotes.

This rule is aware of directive prologues such as `"use strict";` and will not flag or autofix them unless doing so will not change how the directive prologue is interpreted.
Copy link
Member

Choose a reason for hiding this comment

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

A little difficult to parse, maybe:

Suggested change
This rule is aware of directive prologues such as `"use strict";` and will not flag or autofix them unless doing so will not change how the directive prologue is interpreted.
This rule is aware of directive prologues such as `"use strict"` and will not flag or autofix them if doing so will change how the directive prologue is interpreted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I thought that the semicolon in "use strict"; would make it clearer that what is shown is a directive as opposed to just a string, but it that's bad for readability we can leave it out.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, dangling semicolons are a little visually confusing. :)

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @mdjermanovic to review before merging.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 14581ff into main Jun 16, 2023
17 checks passed
@mdjermanovic mdjermanovic deleted the fix-quotes branch June 16, 2023 19:19
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 14, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants