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

A way to override the default ignores #65

Open
jamestalmage opened this issue Jan 3, 2016 · 17 comments · Fixed by #425
Open

A way to override the default ignores #65

jamestalmage opened this issue Jan 3, 2016 · 17 comments · Fixed by #425
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@jamestalmage
Copy link
Contributor

jamestalmage commented Jan 3, 2016

Issuehunt badges

Right now it's not possible to lint anything in a fixtures directory, even if you want to. There should be a way to enable that.

I tried this:

{
  "xo": {
    "ignore": [
      "!{test/,}fixture{s,}/**"
    ]
  }
}

But no luck


IssueHunt Summary

pvdlg pvdlg has been rewarded.

Backers (Total: $80.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Member

I would have thought that would work, and it definitely should. XO comes with good defaults, but everything should be overridable when needed.

@jamestalmage
Copy link
Contributor Author

If anybody wants to take a crack at this #66 has a test prewritten for you!

@jahson
Copy link

jahson commented Jun 27, 2016

@jamestalmage Can you please tell me if ignore in XO config should totally override the default ignore? In current code it's appended to DEFAULT_IGNORE and then passed to globby which in turn passes it to glob.

@jamestalmage
Copy link
Contributor Author

Can you please tell me if ignore in XO config should totally override the default ignore?

No, it should not. Using ignore: '!test/fixtures/**' should not cause XO to start globbing node_modules

@jahson
Copy link

jahson commented Jun 28, 2016

Ok. This complicates things a bit. It looks like we should do some kind of glob expansion to actually override ignores and I doubt it's XO's area of responsibility. Or should it be done before passing to globby?

@abread
Copy link
Contributor

abread commented Oct 5, 2016

I propose this solution:
Negating a default ignore pattern would make xo remove it (and its negation) from the final ignores array.

It's not very flexible because it does no glob expansion, but it is easy to implement and gets the job done. To make it easier to override specific rules, we could just leave the default ignores expanded as such: [ '{tmp,temp}/**' ] -> [ 'tmp/**', 'temp/**' ].

Quick and dirty pseudocode:

// defaultIgnores <- default ignores array
// userIgnores <- the array defined by the user of ignore patterns
// ignores <- final ignores array

for(pattern in defaultIgnores) {
  const index = userIgnores.indexOf('!' + pattern)
  if(index !== -1) {
    delete userIgnores[index]
  } else {
    ignores.push(pattern)
  }
}

ignores = ignores.concat(userIgnores)

In conclusion, it would get xo a bit closer to expected behavior and leave room to reach expected behavior.

@niftylettuce
Copy link

If I specify an ignores: [ ... ] array of Strings to ignore, it gives me this error:

The `ignores` option requires the `filename` option to be defined

novemberborn added a commit to avajs/ava that referenced this issue Sep 24, 2017
Work around xojs/xo#65 by invoking XO
from within the `test/fixture` directory.

Fix linting issues. Most were harmless, but it did turn out that
`babel-plugin-test-doubler.js` no longer doubled tests.
novemberborn added a commit to avajs/ava that referenced this issue Oct 1, 2017
Work around xojs/xo#65 by invoking XO
from within the `test/fixture` directory.

Fix linting issues. Most were harmless, but it did turn out that
`babel-plugin-test-doubler.js` no longer doubled tests.
@tusbar
Copy link

tusbar commented Nov 23, 2017

@sindresorhus I’m having this issue as well. I’m thinking that, instead of doing what @addobandre suggests, we could implement a simple include/exclude (whitelist/blacklist) mechanism that we see everywhere (think webpack loaders, babel plugins, etc.).

Overriding the default ignored files could easily be done by specifying the files you want linted. For example having something like:

{
  "xo": {
    "src": [
      "src/**",
      "index.js",
      "{test/,}fixture{s,}/**"
    ]
  }
}

Specifying src would ignore ignores completely and run xo on the specified glob patterns.

I believe that src or files sound alright as the opposite of ignores, though a plural form is probably better. Whichever you prefer, I’m open to suggestions. Maybe patterns.

Clearly, this can add a lot of lines to package.json but most people probably use the default behavior anyway.

If you’re 👍 with this, I can see and implement it.

tusbar added a commit to tusbar/xo that referenced this issue Nov 23, 2017
This allows to override the default ignored patterns.

Fix xojs#65
@sindresorhus
Copy link
Member

@tusbar I don't like how it would completely opt out of all the default ignores. You would have to define a lot of ignores boilerplate in many projects. I prefer the approach of just individually overriding ignored paths.

@tusbar
Copy link

tusbar commented Nov 25, 2017

The way I see it, there are 3 ways to do this:

1️⃣ What was suggested above: ignore the ignores by negating them.
2️⃣ Completely override the default ignores and specify your own.
3️⃣ Explicitly specify glob patterns to lint.

Consider the following tree:

├── index.js
├── index.min.js
└── fixtures
    ├── success
    │   └── foo.js
    ├── break
    │   └── color.js
    └── vendors
        └── something.js

I want index.js and foo.js to be linted.

Let’s assume that DEFAULT_IGNORE = ['fixtures/**', '**/*.min.js'].

> glob.sync('**', { ignore: ['fixtures/**', '!fixture/success/**'] })
[]

Negating patterns in glob’s ignore option is not a thing, and paths are ignored on first match.

So 1️⃣ is out of the question.

@addobandre suggests that negating a pattern in ignores could remove that pattern from the DEFAULT_IGNORE. It would break whenever the default ignores change, and still require users to define new ignores patterns to replace the removed one.

"xo": {
  "ignores": [
    "!fixtures/**",
    "fixtures/vendors/**",
    "fixtures/break/**"
  ]
}

This feels really awkward.

I would rather override the ignores completely (:two:) and have the following configuration:

"xo": {
  "ignores": [
    "**/*.min.js",
    "fixtures/vendors/**",
    "fixtures/break/**"
  ]
}

As long as the documentation specifies the default ignore patterns, I can port them over to my configuration if I need them.

As you said:

XO comes with good defaults, but everything should be overridable when needed.

In most of the packages that I lint with xo, the default ignores are great, but it gets tedious when you want to lint ignored files (like in ava).

Finally, 3️⃣, I was suggesting a patterns or files option that is mutually exclusive with ignores, where you explicitly specify all the files that will be linted (just like you do when you use the command line).

"xo": {
  "patterns": [
    "index.js",
    "fixtures/success/**"
  ]
}

2️⃣ and 3️⃣ seem like reasonable options, though 2️⃣ is a severe breaking change.

@Qix-
Copy link

Qix- commented Jul 23, 2018

This is kind of annoying - had a script called bundle.js and it was just being silently ignored. Even an explicit xo bundle.js invocation didn't yield anything.

$ mv bundle2.js bundle.js
$ clear
$ echo 'asdfjkaskdjfkasjdf herp derppppppppppp' > bundle.js
$ node_modules/.bin/xo bundle.js
$ mv bundle.js bundle2.js
$ node_modules/.bin/xo bundle2.js

  bundle2.js:1:20
  ✖  1:20  Parsing error: Unexpected token herp

  1 error

Took me 30 minutes to figure out what was going on and that I had a bunch of swallowed linter errors...

Can we re-open #288 as a workaround until upstream Globby gets patched?

@sindresorhus
Copy link
Member

had a script called bundle.js

Hmm, I'm leaning towards just removing bundle.js from default ignores. I've never used that naming myself and it feels too opinionated. The default ignores should be something you would ignore 95% of the time. @pvdlg Thoughts?

Even an explicit xo bundle.js invocation didn't yield anything.

That's a known issue: #238

Can we re-open #288 as a workaround until upstream Globby gets patched?

I would be in favor of just fixing it in Globby since that's easier than generally fixing it in Globby.

@pvdlg Do you remember which Globby issue this is about?

@pvdlg
Copy link
Contributor

pvdlg commented Jul 24, 2018

Hmm, I'm leaning towards just removing bundle.js from default ignores.

Agreed.

I would be in favor of just fixing it in Globby since that's easier than generally fixing it in Globby.

Yes, #288 wasn't the correct fix.

Do you remember which Globby issue this is about?

I can't find any corresponding issue in Globby... I'm not sure what is the correct solution here.
The problem is that both node-glob and fast-glob do not handle ignore globs together. So setting ignore to [!**/bundle.js, **/bundle.js] still ignore **/bundle.js.

My solution was to remove all the negative globs from ignores, reverse them and add them to the list of patterns. I think that I hit some edges cases or performances issues doing so. I can't remember exactly.

sindresorhus added a commit that referenced this issue Jul 28, 2018
It was too opinionated and `bundle.js` can't always be assumed to be generated code that should not be linted. See: #65 (comment)
@sindresorhus
Copy link
Member

Hmm, I'm leaning towards just removing bundle.js from default ignores.

Agreed.

Done: 8e4f435

fast-glob do not handle ignore globs together. So setting ignore to [!**/bundle.js, **/bundle.js] still ignore **/bundle.js.

From https://github.com/mrmlnc/fast-glob/releases/tag/2.2.1:

  • Allow to use negative patterns in the «ignore» option

@IssueHuntBot
Copy link

@issuehuntfest has funded $80.00 to this issue. See it on IssueHunt

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 10, 2019
@issuehunt-oss
Copy link

issuehunt-oss bot commented Feb 17, 2020

@sindresorhus has rewarded $72.00 to @pvdlg. See it on IssueHunt

  • 💰 Total deposit: $80.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $8.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Feb 17, 2020
pvdlg added a commit that referenced this issue Feb 26, 2020
@pvdlg pvdlg reopened this Feb 26, 2020
@pvdlg
Copy link
Contributor

pvdlg commented Feb 26, 2020

Reopening per #435

I sent an email to IssueHunt to revert the reward

sindresorhus pushed a commit that referenced this issue Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants