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

tools: refactor .eslintrc.js #27789

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 21, 2019

tools: edit .eslintrc.js for minor maintainability improvements

* Add a comment explaining the Module._findPath() hacks so that someone
  else doesn't do what I do and try to remove them because they seem
  unnecessary for `make lint` and friends.
* Add a trailing comma for consistency with the rest of the file.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott Trott requested a review from devsnek May 21, 2019 04:17
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 21, 2019
@@ -236,7 +213,7 @@ module.exports = {
{
selector: "CallExpression[callee.property.name='strictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression']):not([arguments.1.type='UnaryExpression'])",
message: 'The first argument should be the `actual`, not the `expected` value.',
}
},
Copy link
Member

Choose a reason for hiding this comment

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

If you want to have a consistent style, what about enabling the rule at the top of the file? That way it would be possible to slowly migrate some files.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

if i remove the _findPath hack eslint still fails for me.

Oops! Something went wrong! :(

ESLint: 5.15.1.
ESLint couldn't find the plugin "eslint-plugin-markdown". This can happen for a couple different reasons:

1. If ESLint is installed globally, then make sure eslint-plugin-markdown is also installed globally. A globally-installed ESLint cannot find a locally-installed plugin.

2. If ESLint is installed locally, then it's likely that the plugin isn't installed correctly. Try reinstalling by running the following:

    npm i eslint-plugin-markdown@latest --save-dev

Path to ESLint package: /Users/gus/.npm-global/lib/node_modules/eslint

If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

meant to do request changes not approve

@Trott
Copy link
Member Author

Trott commented May 23, 2019

@devsnek What are you running to get that error? make lint? make lint-js? Something else?

@Trott
Copy link
Member Author

Trott commented May 24, 2019

@devsnek Are you running a globally-installed eslint? Or maybe on a stale branch? ESLint in our source tree is 5.16.0 but your message say 5.15.1. It hasn't been 5.15.1 since March.

@Trott
Copy link
Member Author

Trott commented May 24, 2019

@devsnek Are you running a globally-installed eslint? Or maybe on a stale branch? ESLint in our source tree is 5.16.0 but your message say 5.15.1. It hasn't been 5.15.1 since March.

Ah, yes, installing eslint globally and trying to run it that way seems to replicate this problem.

Any openness to the idea that perhaps that shouldn't be supported? (I'm unsure myself. I see the benefit of stuff just-working for someone.)

@devsnek
Copy link
Member

devsnek commented May 24, 2019

I think we should definitely be supporting global eslint because that's how editors run linting. lint-js isn't the only usage.

@Trott
Copy link
Member Author

Trott commented May 24, 2019

I think we should definitely be supporting global eslint because that's how editors run linting. lint-js isn't the only usage.

OK! I've left out that change and instead added a comment explaining why the monkeypatching is necessary so no one else comes along and tries to undo it. @devsnek PTAL

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3616/

* Add a comment explaining the Module._findPath() hacks so that someone
  else doesn't do what I do and try to remove them because they seem
  unnecessary for `make lint` and friends.
* Add a trailing comma for consistency with the rest of the file.
@Trott
Copy link
Member Author

Trott commented May 24, 2019

Fixed a tiny thing in the comment. Running Lite CI again...

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3617/

@Trott
Copy link
Member Author

Trott commented May 24, 2019

Landed in 5b8df5e

@Trott Trott closed this May 24, 2019
Trott added a commit to Trott/io.js that referenced this pull request May 24, 2019
* Add a comment explaining the Module._findPath() hacks so that someone
  else doesn't do what I do and try to remove them because they seem
  unnecessary for `make lint` and friends.
* Add a trailing comma for consistency with the rest of the file.

PR-URL: nodejs#27789
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit that referenced this pull request May 28, 2019
* Add a comment explaining the Module._findPath() hacks so that someone
  else doesn't do what I do and try to remove them because they seem
  unnecessary for `make lint` and friends.
* Add a trailing comma for consistency with the rest of the file.

PR-URL: #27789
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@targos targos mentioned this pull request Jun 3, 2019
@Trott Trott deleted the refactor-eslintrc.js branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants