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

eslint with --rule option not working properly #419

Comments

@tgreen7
Copy link

tgreen7 commented Mar 30, 2018

Steps to reproduce

Here is a example repo: https://github.com/tgreen7/lint-staged-eslint-rule

if you modify index.js, git add index.js, and run yarn precommit then no errors will be logged. However, if you uncomment the line quotes: [2, 'double'] in the .eslintrc.js file and re-run the precommit then the error will be logged.

Also you can try to run yarn lint, which adds that rule in the same way as the precommit, and it does get linted properly.

If I am passing that same rule to the precommit through the eslint --rule option then why is it not getting picked up while running?

Debug Logs

expand to view
[taoh:.../lint-staged-test] ❯ ./node_modules/.bin/lint-staged --debug                                                   ✚ :: master  :: ⬡
  lint-staged:bin Running `lint-staged@7.0.0` +0ms
  lint-staged Loading config using `cosmiconfig` +0ms
  lint-staged Successfully loaded config from `/Users/taoh/Sites/lint-staged-test/package.json`:
  lint-staged { 'index.js': [ 'eslint --rule \'quotes: [2, double]\'' ] } +3ms
  lint-staged:cfg Normalizing config +0ms
  lint-staged:cfg Validating config +2ms
Running lint-staged with the following config:
{
  linters: {
    'index.js': [
      'eslint --rule \'quotes: [2, double]\''
    ]
  },
  concurrent: true,
  chunkSize: 9007199254740991,
  globOptions: {
    matchBase: true,
    dot: true
  },
  ignore: [],
  subTaskConcurrency: 1,
  renderer: 'verbose'
}
  lint-staged:run Running all linter scripts +0ms
  lint-staged:run Resolved git directory to be `/Users/taoh/Sites/lint-staged-test` +1ms
  lint-staged:run Loaded list of staged files in git:
  lint-staged:run [ 'index.js' ] +28ms
  lint-staged:gen-tasks Generating linter tasks +0ms
  lint-staged:cfg Normalizing config +35ms
  lint-staged:gen-tasks Generated task:
  lint-staged:gen-tasks { pattern: 'index.js',
  lint-staged:gen-tasks   commands: [ 'eslint --rule \'quotes: [2, double]\'' ],
  lint-staged:gen-tasks   fileList: [ '/Users/taoh/Sites/lint-staged-test/index.js' ] } +2ms
Running tasks for index.js [started]
  lint-staged:run-script Running script with commands [ 'eslint --rule \'quotes: [2, double]\'' ] +0ms
  lint-staged:cfg Normalizing config +5ms
eslint --rule 'quotes: [2, double]' [started]
  lint-staged:find-bin Resolving binary for command `eslint --rule 'quotes: [2, double]'` +0ms
  lint-staged:find-bin Binary for `eslint --rule 'quotes: [2, double]'` resolved to `/Users/taoh/Sites/lint-staged-test/node_modules/.bin/eslint` +3ms
  lint-staged:run-script bin: /Users/taoh/Sites/lint-staged-test/node_modules/.bin/eslint +5ms
  lint-staged:run-script args: [ '--rule',
  lint-staged:run-script   '\'quotes:',
  lint-staged:run-script   '[2,',
  lint-staged:run-script   'double]\'',
  lint-staged:run-script   '/Users/taoh/Sites/lint-staged-test/index.js' ] +0ms
  lint-staged:run-script opts: { reject: false } +1ms
eslint --rule 'quotes: [2, double]' [completed]
Running tasks for index.js [completed]
  lint-staged linters were executed successfully! +549ms

Environment

  • OS: macOS High Sierra
  • Node.js: v9.8.0
  • lint-staged: v7.0.0
@sudo-suhas
Copy link
Collaborator

@tgreen7 This might be an eslint issue:

λ yarn lint
yarn run v1.5.1
$ eslint --rule 'quotes: [2, double]' index.js
Done in 1.28s.

λ .\node_modules\.bin\eslint.cmd --rule 'quotes: [2, double]' index.js
<no errors>

# Uncomment the rule in .eslint.js
λ .\node_modules\.bin\eslint.cmd index.js

E:\Projects\experiments\lint-staged-eslint-rule\index.js
  1:9  error  Strings must use doublequote  quotes

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

To be completely honest, this could also be hiding a lint-staged bug because I am not sure the way we split your command into arguments is completely correct when single quotes are present.

@okonet
Copy link
Collaborator

okonet commented Mar 31, 2018

@sudo-suhas it looks like a bug on our side, TBH. From logs it seems we should not split arguments if they are quoted.

  lint-staged:run-script args: [ '--rule',
  lint-staged:run-script   '\'quotes:',
  lint-staged:run-script   '[2,',
  lint-staged:run-script   'double]\'',
  lint-staged:run-script   '/Users/taoh/Sites/lint-staged-test/index.js' ]

So in this case it should be only 3 arguments instead of 5. Am I right?

@okonet
Copy link
Collaborator

okonet commented Mar 31, 2018

So it seems this one https://github.com/okonet/lint-staged/blob/master/src/findBin.js#L26 is a very naïve implementation. We should probably switch to something more robust.

@sudo-suhas
Copy link
Collaborator

@okonet yes, that is a possibility. I say possibility because execa might be handling the arguments being split on single quotes. We should verify that first. However, in this specific case the eslint command, even when run by itself, is not doing what we expect.

@okonet
Copy link
Collaborator

okonet commented Mar 31, 2018

@sudo-suhas not sure what you mean. It works for me on this repo:

➜ eslint --rule 'quotes: [2, double]' index.js 

/Users/okonet/Projects/OSS/lint-staged/index.js
   3:1   error  Strings must use doublequote  quotes
   5:25  error  Strings must use doublequote  quotes
   6:26  error  Strings must use doublequote  quotes
   7:21  error  Strings must use doublequote  quotes
   9:24  error  Strings must use doublequote  quotes
  13:11  error  Strings must use doublequote  quotes
  13:34  error  Strings must use doublequote  quotes
  14:11  error  Strings must use doublequote  quotes
  14:26  error  Strings must use doublequote  quotes
  18:19  error  Strings must use doublequote  quotes
  21:7   error  Strings must use doublequote  quotes
  23:9   error  Strings must use doublequote  quotes

✖ 12 problems (12 errors, 0 warnings)
  12 errors, 0 warnings potentially fixable with the `--fix` option.

@okonet
Copy link
Collaborator

okonet commented Mar 31, 2018

@tgreen7 BTW thanks for the pointer! I just realized how to use this option to better integrate prettier with eslint so it's less annoying during dev 👍

@sudo-suhas
Copy link
Collaborator

not sure what you mean. It works for me on this repo:

It was a windows specific problem:

λ .\node_modules\.bin\eslint.cmd --rule 'quotes: [2, double]' index.js

λ .\node_modules\.bin\eslint.cmd --rule "quotes: [2, double]" index.js

E:\Projects\experiments\lint-staged-eslint-rule\index.js
  1:9  error  Strings must use doublequote  quotes

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

@okonet
Copy link
Collaborator

okonet commented Apr 1, 2018

Is it still a problem?

@sudo-suhas
Copy link
Collaborator

The PR you created in #422 fixes it.

@tgreen7
Copy link
Author

tgreen7 commented Apr 4, 2018

@okonet I am still seeing this issue even after the merge of cli-command-parser

@okonet
Copy link
Collaborator

okonet commented Apr 4, 2018

Not sure why. It works for me. See the PR which integrates it.

@tgreen7
Copy link
Author

tgreen7 commented Apr 4, 2018

It looks like the args are parsed in the same way and I do have the cli-command-parser package

Debug Details
[taoh:...taged-eslint-rule] ❯ ./node_modules/.bin/lint-staged --debug                                                      ✹✚ :: master  :: ⬡
  lint-staged:bin Running `lint-staged@7.0.3` +0ms
  lint-staged Loading config using `cosmiconfig` +0ms
  lint-staged Successfully loaded config from `/Users/taoh/Sites/lint-staged-eslint-rule/package.json`:
  lint-staged { 'index.js': [ 'eslint --rule \'quotes: [2, double]\'' ] } +4ms
  lint-staged:cfg Normalizing config +0ms
  lint-staged:cfg Validating config +2ms
Running lint-staged with the following config:
{
  linters: {
    'index.js': [
      'eslint --rule \'quotes: [2, double]\''
    ]
  },
  concurrent: true,
  chunkSize: 9007199254740991,
  globOptions: {
    matchBase: true,
    dot: true
  },
  ignore: [],
  subTaskConcurrency: 1,
  renderer: 'verbose'
}
  lint-staged:run Running all linter scripts +0ms
  lint-staged:run Resolved git directory to be `/Users/taoh/Sites/lint-staged-eslint-rule` +1ms
  lint-staged:run Loaded list of staged files in git:
  lint-staged:run [ 'index.js' ] +22ms
  lint-staged:gen-tasks Generating linter tasks +0ms
  lint-staged:cfg Normalizing config +26ms
  lint-staged:gen-tasks Generated task:
  lint-staged:gen-tasks { pattern: 'index.js',
  lint-staged:gen-tasks   commands: [ 'eslint --rule \'quotes: [2, double]\'' ],
  lint-staged:gen-tasks   fileList: [ '/Users/taoh/Sites/lint-staged-eslint-rule/index.js' ] } +2ms
Running tasks for index.js [started]
  lint-staged:run-script Running script with commands [ 'eslint --rule \'quotes: [2, double]\'' ] +0ms
  lint-staged:cfg Normalizing config +5ms
eslint --rule 'quotes: [2, double]' [started]
  lint-staged:find-bin Resolving binary for command `eslint --rule 'quotes: [2, double]'` +0ms
  lint-staged:find-bin Binary for `eslint --rule 'quotes: [2, double]'` resolved to `/Users/taoh/Sites/lint-staged-eslint-rule/node_modules/.bin/eslint` +2ms
  lint-staged:run-script bin: /Users/taoh/Sites/lint-staged-eslint-rule/node_modules/.bin/eslint +5ms
  lint-staged:run-script args: [ '--rule',
  lint-staged:run-script   '\'quotes:',
  lint-staged:run-script   '[2,',
  lint-staged:run-script   'double]\'',
  lint-staged:run-script   '/Users/taoh/Sites/lint-staged-eslint-rule/index.js' ] +0ms
  lint-staged:run-script opts: { reject: false } +0ms
eslint --rule 'quotes: [2, double]' [completed]
Running tasks for index.js [completed]
  lint-staged linters were executed successfully! +414ms
...taged-eslint-rule] ❯ grep cli-command-parser node_modules/lint-staged/src/*                                       ✹✚ :: master  :: ⬡
node_modules/lint-staged/src/findBin.js:const parse = require('cli-command-parser')

@okonet
Copy link
Collaborator

okonet commented Apr 4, 2018

@tgreen7 I can confirm this when using single quotes. I'll reopen and try to address it. Meanwhile you can use double quotes.

@okonet okonet reopened this Apr 4, 2018
okonet pushed a commit that referenced this issue Apr 4, 2018
Switched to string-argv since cli-command-parser seems to have a bug.

Closes #419
@tgreen7
Copy link
Author

tgreen7 commented Apr 4, 2018

Thanks! Double quotes works 👍

okonet pushed a commit that referenced this issue Apr 5, 2018
Switched to string-argv since cli-command-parser seems to have a bug.

Closes #419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment