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

Add defaultIgnores: false option #2382

Closed
wants to merge 1 commit into from
Closed

Conversation

hudochenkov
Copy link
Member

Which issue, if any, is this issue related to?

#2236

Is there anything in the PR that needs further explanation?

Changing default behaviour for ignoreFiles. User's config will override default values.

Should we somewhere in docs write default pattern? Users might want to add some patterns additionally to default patterns.

Also, what to do to this code which was added recently (9578c48).

@davidtheclark
Copy link
Contributor

I think there's a significant problem here: #2399 (comment). The gist of it is that I can't think of an efficient way for ignoreFiles to work on large quantities of files that match the original input glob. A file will only be ignored via ignoreFiles after stylelint has loaded its relevant config and checked that config for ignoreFiles — which means using ignoreFiles to ignore node_modules will be significantly less efficient than the current implement @hudochenkov linked to.

@hudochenkov
Copy link
Member Author

On the other side this code made impossible #2236. It ignores files which might not be ignored if ignoreFiles are specified.

@davidtheclark
Copy link
Contributor

Uuuggggh.

Here's the only decent solution I can come up with for this mess:

  • config.ignoreFiles comes with the caveat that it is not an efficient way to ignore a lot of files, because it is included in configs and configs are loaded for each file before they can be analyzed — so we don't know if the file is ignored until we've loaded its config.
  • If you want to ignore lots of files efficiently, you need to do that in your input glob.
  • node_modules and bower_components are by default ignored via the input glob (so, efficiently); and we can add a new option like defaultIgnores: false that turns this behavior off.

@hudochenkov
Copy link
Member Author

Sorry, I don't have enough knowledge about ignoring files, and stylelint's architecture. I can't have strong opinion on that matter. I believe other team members could have a solution.

@TheXardas
Copy link

Gentlemen,
maybe a simple flag like "disableNodeModulesIgnore" would do?

@jeddy3
Copy link
Member

jeddy3 commented Apr 1, 2017

Here's the only decent solution I can come up with for this mess:

SGTM. Let's document the limitation of config.ignoreFiles and how a more tailored input glob can be used as an alternative, and let's add a defaultIgnores: false option.

Am I right in thinking that such an option would no longer mean that this is a breaking change and we can point this PR (once it's adapted) to master?

maybe a simple flag like "disableNodeModulesIgnore" would do?

@TheXardas Thanks for chiming in. Yep, that is part of @davidtheclark's proposal.

@jeddy3 jeddy3 changed the title Default ignoreFiles value will be overridden by user's config Add defaultIgnores: false option Apr 1, 2017
@davidtheclark davidtheclark self-assigned this Apr 1, 2017
@davidtheclark
Copy link
Contributor

@jeddy3, I think I've realized a key here (#2399 (comment)): You can already load only one .stylelintignore — so we should change our strategy a bit so those patterns get incorporated into the glob. ignoreFiles will be the inefficient way to ignore things; .stylelintignore or your files glob the efficient way.

This will be a breaking change, though the break probably will not affect most users: the ignored and standaloneIgnored properties of stylelint results will no longer be the same, and files that are ignored via .stylelintignore will not show up in the results at all.

I'll try to prepare a PR to v8 today.

@jeddy3
Copy link
Member

jeddy3 commented Apr 1, 2017

I think I've realized a key here (#2399 (comment)):

I saw that. SGTM.

I'll try to prepare a PR to v8 today.

Awesome! :)

@jeddy3
Copy link
Member

jeddy3 commented Apr 3, 2017

Closing in favour of #2464

@jeddy3 jeddy3 closed this Apr 3, 2017
@jeddy3 jeddy3 deleted the ignore-files-defaults branch April 3, 2017 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants