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

Allow all directives in line comments #14575

Closed
mdjermanovic opened this issue May 7, 2021 · 11 comments · Fixed by #14656
Closed

Allow all directives in line comments #14575

mdjermanovic opened this issue May 7, 2021 · 11 comments · Fixed by #14656
Assignees
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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features

Comments

@mdjermanovic
Copy link
Member

This is an issue to track the development of eslint/rfcs#34.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible labels May 7, 2021
@mdjermanovic mdjermanovic self-assigned this May 7, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage May 7, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Ready to Implement in Triage May 7, 2021
@mdjermanovic mdjermanovic added this to Planned in v8.0.0 May 7, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 1, 2021
@aladdin-add
Copy link
Member

aladdin-add commented Jun 1, 2021

I made a simple implementation, and found, in eslint codebase, ~41 inline comments which were not intended as a directive, but parse as directives.

➜  eslint git:(issue14575) npm run lint

> eslint@7.27.0 lint
> node Makefile.js lint

Validating JavaScript files

/Users/weiran/repo/github/eslint/lib/rules/newline-before-return.js
  137:51  error  'return' is defined but never used     no-unused-vars
  137:58  error  'at' is defined but never used         no-unused-vars
  137:61  error  'beginning' is defined but never used  no-unused-vars
  137:71  error  'of' is defined but never used         no-unused-vars
  137:74  error  'script' is defined but never used     no-unused-vars

/Users/weiran/repo/github/eslint/tests/lib/rules/id-blacklist.js
  1013:20  error  'declared' is defined but never used       no-unused-vars
  1013:29  error  'in' is defined but never used             no-unused-vars
  1013:32  error  'the' is defined but never used            no-unused-vars
  1013:36  error  'given' is defined but never used          no-unused-vars
  1013:42  error  'source' is defined but never used         no-unused-vars
  1013:49  error  'code' is defined but never used           no-unused-vars
  1013:54  error  'are' is defined but never used            no-unused-vars
  1013:58  error  'not' is defined but never used            no-unused-vars
  1013:62  error  'excluded' is defined but never used       no-unused-vars
  1013:71  error  'from' is defined but never used           no-unused-vars
  1013:76  error  'consideration' is defined but never used  no-unused-vars

/Users/weiran/repo/github/eslint/tests/lib/rules/id-denylist.js
  1013:20  error  'declared' is defined but never used       no-unused-vars
  1013:29  error  'in' is defined but never used             no-unused-vars
  1013:32  error  'the' is defined but never used            no-unused-vars
  1013:36  error  'given' is defined but never used          no-unused-vars
  1013:42  error  'source' is defined but never used         no-unused-vars
  1013:49  error  'code' is defined but never used           no-unused-vars
  1013:54  error  'are' is defined but never used            no-unused-vars
  1013:58  error  'not' is defined but never used            no-unused-vars
  1013:62  error  'excluded' is defined but never used       no-unused-vars
  1013:71  error  'from' is defined but never used           no-unused-vars
  1013:76  error  'consideration' is defined but never used  no-unused-vars

/Users/weiran/repo/github/eslint/tests/lib/rules/no-promise-executor-return.js
   89:19  error  'Promise' is already defined as a built-in global variable  no-redeclare
   89:19  error  'Promise' is defined but never used                         no-unused-vars
   89:27  error  'doesn't' is defined but never used                         no-unused-vars
   89:35  error  'exist' is defined but never used                           no-unused-vars
  100:19  error  'Promise' is already defined as a built-in global variable  no-redeclare
  100:27  error  'is' is defined but never used                              no-unused-vars
  100:30  error  'shadowed' is defined but never used                        no-unused-vars

/Users/weiran/repo/github/eslint/tests/lib/rules/no-setter-return.js
  227:19  error  'object' is defined but never used    no-unused-vars
  227:26  error  'doesn't' is defined but never used   no-unused-vars
  227:34  error  'exist' is defined but never used     no-unused-vars
  235:19  error  'object' is already defined           no-redeclare
  235:26  error  'is' is defined but never used        no-unused-vars
  235:29  error  'shadowed' is defined but never used  no-unused-vars

/Users/weiran/repo/github/eslint/tests/lib/rules/require-await.js
  46:19  error  'await' is defined but never used  no-unused-vars

✖ 41 problems (41 errors, 0 warnings)

I do know the rfc has been accepted, but is there a possibility to reconsider the issue?( as the concern in the rfc is not fully addressed.)

@mdjermanovic
Copy link
Member Author

I made a simple implementation, and found, in eslint codebase, ~41 inline comments which were not intended as a directive, but parse as directives.

I noticed the same, but it's actually 8 comments causing 41 errors (each comment defines multiple variables).

I do know the rfc has been accepted, but is there a possibility to reconsider the issue?( as the concern in the rfc is not fully addressed.)

It was addressed in Backwards Compatibility Analysis, by treating this as a breaking change.

@mdjermanovic
Copy link
Member Author

@aladdin-add are you already working on this? I was about to start writing code, but if you've already started you can take it.

@aladdin-add
Copy link
Member

fine, I will take it, will make a PR in 1~2 days. thanks! ❤️

@mdjermanovic mdjermanovic moved this from Planned to Pull Request Opened in v8.0.0 Jun 2, 2021
@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Jun 2, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 5, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 11, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 11, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 12, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 18, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 18, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jul 14, 2021
Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex
Triage automation moved this from Pull Request Opened to Complete Jul 14, 2021
@aladdin-add aladdin-add reopened this Jul 14, 2021
Triage automation moved this from Complete to Evaluating Jul 14, 2021
@aladdin-add
Copy link
Member

oops.. hit the wrong button. sorry!

@nzakas nzakas moved this from Evaluating to Pull Request Opened in Triage Jul 15, 2021
@btmills btmills moved this from Pull Request Opened to Ready for Merge in v8.0.0 Jul 15, 2021
Triage automation moved this from Pull Request Opened to Complete Aug 5, 2021
v8.0.0 automation moved this from Ready for Merge to Done Aug 5, 2021
nzakas pushed a commit that referenced this issue Aug 5, 2021
* Breaking: allow all directives in line comments (fixes #14575)

Chore: fix linting problems

Update linter.js

Update linter.js

Update: support eslint-env

fix: rm "\n" in the regex

* Update lib/linter/linter.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* chore: add more tests

* chore: rename var to mustBeOnSingleLine

* fix: false postives of no-inline-comments & no-warning-comments

* Update lib/rules/utils/ast-utils.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update ast-utils.js

* chore: add some tests

* chore: move a var def to outer scope

* chore: add //eslint-enable tests

* chore: add more tests for //eslint-env

* Update lib/rules/utils/ast-utils.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update tests/lib/rules/no-warning-comments.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* chore: add more tests (+1 squashed commit)
Squashed commits:
[9a4c7d7] chore: add more tests

* docs: update config comments usage

* Update ast-utils.js

* Update ast-utils.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Aug 24, 2021
mdjermanovic added a commit that referenced this issue Aug 27, 2021
* Revert "Breaking: allow all directives in line comments (fixes #14575) (#14656)"

This reverts commit 4c841b8.

* fix: codeql warning

refs: https://github.com/eslint/eslint/pull/14973/checks?check_run_id=3411736426

* chore: improve regex

* Update lib/linter/linter.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@aladdin-add aladdin-add reopened this Sep 1, 2021
Triage automation moved this from Complete to Evaluating Sep 1, 2021
@aladdin-add
Copy link
Member

reopened, as it has been reverted in 41617ec.

@nzakas nzakas removed this from Done in v8.0.0 Sep 2, 2021
@nzakas nzakas added this to Need Discussion in v9.0.0 Sep 2, 2021
@glen-84
Copy link

glen-84 commented Jan 8, 2022

Could it be added for eslint-disable and eslint-enable, at least?

I would pair this with the deprecation and later removal of global, export, and eslint, which are far too generic for directives (to be replaced by prefixed versions).

@mdjermanovic
Copy link
Member Author

Related discussion: #14960

@nzakas
Copy link
Member

nzakas commented Aug 14, 2023

TSC Summary: This accepted proposal is to allow all inline directives in line comments in addition to block comments. When we tried to implement this, the result was fairly disruptive to the ecosystem. This issue has say open for over a year without any progress.

TSC Question: Should we close that as "won't fix" now that we know the result when we tried to do this last?

@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 14, 2023
@glen-84
Copy link

glen-84 commented Aug 14, 2023

I hope that the TSC will consider my suggestion of prefixing comment directives, in order to avoid ambiguity.

exported -> eslint-exported
globals -> eslint-globals
global -> eslint-global
eslint -> eslint-rules

(or a syntax even less likely to be part of a non-directive comment, like eslint::exported)

This would be consistent with other directives like eslint-enable, and avoid the issue that forced the initial revert.

@sam3k sam3k closed this as completed Aug 26, 2023
@sam3k sam3k removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 26, 2023
@sam3k
Copy link
Contributor

sam3k commented Aug 26, 2023

Per the decision made in the 2024-08-24 tsc-meeting, we are closing this issue and won't be moving forward with it.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 23, 2024
@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 Feb 23, 2024
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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features
Projects
Archived in project
Triage
Evaluating
v9.0.0
Need Discussion
Development

Successfully merging a pull request may close this issue.

5 participants