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

v1.10.0 breaks command line `--ignore-pattern` behavior #4507

Closed
sanemat opened this Issue Nov 21, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@sanemat
Copy link

sanemat commented Nov 21, 2015

v1.9.0 and earlier accepts command line option --ignore-pattern with {} like --ignore-pattern "foo-{bar,baz}.js".
But v1.10.0 or newer does not accept --ignore-pattern "foo-{bar,baz}.js".
Both v1.9.0 and v1.10.0 accepts --ignore-pattern "foo-bar.js".

Is this expected behavior?

# with v1.10.0
$ npm run pattern:a

> @ pattern:a /Users/sane/work/tmp/eslint-ignore-pattern/not-work
> eslint --ignore-pattern "raise-{warning,foo}.js" .


/Users/sane/work/tmp/eslint-ignore-pattern/not-work/raise-warning.js
  1:5  error  "foo" is defined but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)

reproducing repository

https://github.com/sanemat/eslint-ignore-pattern

$ git ls-files
.gitignore
.gitkeep
latest/.eslintrc
latest/package.json
latest/raise-warning.js
not-work/.eslintrc
not-work/package.json
not-work/raise-warning.js
work/.eslintrc
work/package.json
work/raise-warning.js
$ cat not-work/package.json 
{
  "devDependencies": {
    "eslint": "1.10.0"
  },
  "scripts": {
    "pattern:a": "eslint --ignore-pattern \"raise-{warning,foo}.js\" .",
    "pattern:b": "eslint --ignore-pattern \"raise-warning.js\" ."
  }
}
my env:
osx 10.9.5
nodejs 4.2.1
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 21, 2015

A change was made to allow more than one pattern to be passed, but that shouldn't have caused such a change. Definitely a bug.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Nov 23, 2015

Update:

I think its a problem with optionator project

if this is the argument provide "eslint --ignore-pattern \"raise-{warning,foo}.js\" ."
Optionator creates an array of ignore patterns in this format

[
"raise-{warning",
"foo}.js"
]

Since we switched the option definition from string to [string] array of string

@nzakas nzakas added the blocked label Nov 23, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 23, 2015

@gkz is there any way to avoid splitting the option value on the comma?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 23, 2015

@sanemat while we are trying to get this straightened out, you can work around the issue by putting a slash before the comma (",").

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Nov 23, 2015

I dont think backslash before the comma \,would fix the problem. What you can do right now is

eslint --ignore-pattern \"raise-warning.js\" --ignore-pattern \"raise-foo.js\" .
@gkz

This comment has been minimized.

Copy link
Contributor

gkz commented Nov 24, 2015

@nzakas I'll look into it

@gkz

This comment has been minimized.

Copy link
Contributor

gkz commented Nov 26, 2015

I can fix the problem, but it would mean that you wouldn't be able to create an array with --ignore-pattern a,b, but would need to use --ignore-pattern twice, eg. --ignore-pattern a --ignore-pattern b - since it can't know whether the comma is for delimiting values, or just part of your value. Is that acceptable?

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Nov 26, 2015

Thats fine, as that how we accept to form an array in this option as well as for other options.
Thanks @gkz for looking into this.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 26, 2015

@gkz sounds good. Would it be possible to apply that just to this one option? Or would it need to be for all options?

@gkz

This comment has been minimized.

Copy link
Contributor

gkz commented Dec 1, 2015

I plan on making it a per-option option, so it would only affect --ignore-pattern.
This week is the last week of the term, so if you guys can wait one week for the fix, that would be great. If it is super urgent, let me know and I'll try to do something earlier.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 1, 2015

We can wait, thanks!

ilyavolodin added a commit that referenced this issue Dec 7, 2015

Merge pull request #4547 from platinumazure/ignore-pattern-array-tests
Fix: Adding options unit tests for --ignore-pattern (refs #4507)

gkz added a commit to gkz/optionator that referenced this issue Dec 8, 2015

add concat-repeated-array opt: one-value-per-flag
Only one value per flag, repeat flags for multiple values, creates array
eslint/eslint#4507

@gyandeeps gyandeeps removed the blocked label Dec 9, 2015

@nzakas nzakas closed this in f06d6a6 Dec 9, 2015

nzakas added a commit that referenced this issue Dec 9, 2015

Merge pull request #4647 from gkz/issue4507
Fix: Use oneValuePerFlag for --ignore-pattern option (fixes #4507)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.