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

Fix: Allow linting of .hidden files/folders (fixes #4828) #6844

Merged
merged 1 commit into from Sep 8, 2016

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Aug 5, 2016

What issue does this pull request address?
#4828

What changes did you make? (Give an overview)
The main change here is to set an option on node_glob to return dotfiles from glob patterns (anything other than explicitly passed files). In order for dotfiles to actually be linted, though, a negating ignore pattern like !.* will need to be used to override the default ignore behavior.

This turned out to be a bit more involved than just returning hidden files from our glob patterns. That's because we were not ignoring dotfiles by default in all cases, only when an options object was provided to listFilesToProcess, which was nearly always. Except, it turns out, in the test "should not return hidden files for standard glob patterns". That was okay, because globs would never return dotfiles, but that started failing as soon as I made the core change in this PR.

Now that the glob pattern will include hidden files, we need to apply the default ignore rules. However, we also want to not ignore dotfiles if they are explicitly included in the glob pattern. The only way I could find to solve this problem was to directly check the glob pattern with a regex to see if it includes hidden files. If so, dotfiles are not ignored, and otherwise they are.

Is there anything you'd like reviewers to focus on?
Please give my regex a sanity check. I tried to test it out and it seems to work, but I'm not super-confident in my regex skills.

I would also be happy to include any other tests that anyone thinks might be useful.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

// regex to find .hidden or /.hidden patterns, but not ./relative or ../relative
const globIncludesDotfiles = /(?:(?:^\.)|(?:[\/\\]\.))[^\/\\\.].*/.test(pattern);

options.dotfiles = options.dotfiles || globIncludesDotfiles;
Copy link
Member

Choose a reason for hiding this comment

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

Should this option (dotfiles) be documented somewhere? How can it be set outside of addFile function?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, it's internal only. It's been there since we switched over to node-glob, in fact. (https://github.com/eslint/eslint/blob/master/lib/ignored-paths.js#L113)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... We seemed to leaked a bunch of undocumented features into our options:-) Ok, another question then, would this start linting dotfiles automatically, or does user needs to do something to include them?

Copy link
Member Author

@IanVS IanVS Aug 9, 2016

Choose a reason for hiding this comment

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

Dotfiles/dotfolders will not be linted in most cases, only if the user explicitly does one of the following:

  1. specifies a glob which would include dotfiles (e.g. .* or test/**/.*) (this is what the regex I added is intended to determine)
  2. unignores dotfiles with an --ignore-pattern or pattern in an .eslintignore file. This is similar to our other default ignores of node_modules and bower_components

Copy link
Member

Choose a reason for hiding this comment

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

Can you add some documentation explaining this behavior? It's still unclear to me under what circumstances a dot file will be linted and when it will not. If it's not clear to us on the team, then we definitely need this to be explicitly described somewhere for end users.

@nzakas
Copy link
Member

nzakas commented Aug 11, 2016

As mentioned in my other comment, I think we need some good documentation around this behavior as I don't think it's clear on its own. Specifically, we need to be clear about:

  1. If a dot file is passed on the command line, is it always linted? (Hopefully yes.)
  2. If I run eslint ., will dot files be linted?
  3. If I run eslint .*, will dot files be linted?
  4. If dot files aren't linted, what do I do to make them linted?

@IanVS
Copy link
Member Author

IanVS commented Aug 11, 2016

Will do. 👍

On Thu, Aug 11, 2016, 12:57 Nicholas C. Zakas notifications@github.com
wrote:

As mentioned in my other comment, I think we need some good documentation
around this behavior as I don't think it's clear on its own. Specifically,
we need to be clear about:

  1. If a dot file is passed on the command line, is it always linted?
    (Hopefully yes.)
  2. If I run eslint ., will dot files be linted?
  3. If I run eslint .*, will dot files be linted?
  4. If dot files aren't linted, what do I do to make them linted?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#6844 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AEZyAY9rlym8t90XOZM3Z5MNclgbqTuSks5qe1SGgaJpZM4JdVIj
.

@pdehaan
Copy link
Contributor

pdehaan commented Aug 11, 2016

I recently hit this while trying to figure out why ESLint wasn't linting any of the files in a React ./.storybook/* directory. This PR and comments were excellent in helping me figure everything out.

Basically we had to modify .eslintrc to include !.* and then explicitly tell ESLint to lint . and .storybook:

"test:lint": "eslint . .storybook",

Thanks again for your excellent work on ESLint!

@IanVS
Copy link
Member Author

IanVS commented Aug 13, 2016

@nzakas here are a few answers. I will find good places to document this in code as well as in the docs, but that will take a little more time.

  1. If a dot file is passed on the command line, is it always linted? (Hopefully yes.)

This behavior is unchanged by this PR. If a dotfile is passed on the command line, it is treated as any other file. If it is ignored, a warning is shown. If it is ignored by default, the warning is File ignored by default. Use a negated ignore pattern (like "--ignore-pattern '!<relative/path/to/filename>'") to override. We can talk about whether this is the right behavior, but again, it is unchanged by this PR.

  1. If I run eslint ., will dot files be linted?

No, again that is unchanged here. (although see the answer to 4. below)

  1. If I run eslint .*, will dot files be linted?

That depends on the user's shell. If they are using something like zsh, the glob is expanded to a list of files before eslint ever sees it. In that case, this PR does not change the behavior of master, and is exactly like 1. above.

If, however, the shell does not expand, or the user adds double quotes around the glob (eslint ".*"), then this PR does change behavior. Previously, all dotfiles would be ignored without warning. Now, any dotfiles matching the glob will be linted.

  1. If dot files aren't linted, what do I do to make them linted?

This is the other change in this PR. As I said above, eslint . will not lint dotfiles. Currently, eslint . --ignore-pattern '!.*' will also not lint dotfiles. In other words, the ignore pattern doesn't work. With this PR, that ignore pattern will unignore all dotfiles, causing them to be linted. Of course, the same principle applies if the pattern is added to a .eslintignore file.

I hope this makes it a bit clearer for the team as to the current behavior, as well as the purpose of this PR. As I said, I will try to simplify and clean this up and get it into docs if it's agreed that this is the right approach. I also have a few other small tweaks to make, but I want to wait until #6783 is merged because I'm going to have some merge conflicts.

@ilyavolodin
Copy link
Member

@IanVS Just to make sure I fully understand. If you lint using any pattern that specifically includes dot-files, dot-files will not be linted, message that they are skipped will not be displayed. If you want to lint dot-files you have to do one of 2 things:
Use a pattern that will include dot-files OR
Un-ignore dot files either on command line or through .eslintignore file.

Is that correct?

@IanVS
Copy link
Member Author

IanVS commented Aug 14, 2016

@ilyavolodin sorry, can you be a bit more specific? When you say "any pattern that specifically includes dot-files", do you mean a glob (one that is not shell-expanded) like eslint "**/.*.js"? Also, are you asking about the current behavior, or the behavior of this PR?

@ilyavolodin
Copy link
Member

@IanVS Yes, a glob like the one you provided. And I'm asking about this PR.

@IanVS
Copy link
Member Author

IanVS commented Aug 14, 2016

With this PR and that pattern, all matching dotfiles will be linted. Essentially, the default ignore of dotfiles is turned off if a glob pattern specifying dotfiles or dotfolders is given. Other ignore rules from .eslintignore or --ignore-pattern are still respected and files matching those are silently ignored (as before).

@nzakas
Copy link
Member

nzakas commented Aug 24, 2016

Sorry, lost track of this.

The one thing that I'm not sure is the behavior we want is passing a dot file on the command line. It seems like this should lint the file:

eslint .eslintrc.js

I thought we had changed this behavior before so that explicitly passed files would always be linted regardless of ignore settings?

@IanVS
Copy link
Member Author

IanVS commented Aug 24, 2016

I thought we had changed this behavior before so that explicitly passed files would always be linted regardless of ignore settings?

Not to my knowledge, and that's definitely not the current behavior. And actually, changing that behavior would effect other tools like atom's eslint plugin, as dotfiles would start being linted when opened whereas they currently are not. I'm not saying it wouldn't necessarily be the right thing to do, but I do personally think it should be addressed in a separate issue/PR and have some discussion first.

@IanVS
Copy link
Member Author

IanVS commented Aug 24, 2016

BTW, I will rebase this with master tonight.

@eslintbot
Copy link

LGTM

@IanVS
Copy link
Member Author

IanVS commented Aug 25, 2016

I've rebased onto master.

@IanVS
Copy link
Member Author

IanVS commented Aug 25, 2016

@nzakas part of the reason I am reluctant to "lint anything passed explicitly" is that it is a more difficult challenge to solve. There is not always an easy distinction between "explicitly passed" and "expanded from a glob by the user's shell". I think this PR gives some "quick" wins towards being able to lint dotfiles, and there will need to be a larger re-architecture to be able to always lint directly specified files.

@ilyavolodin
Copy link
Member

Hmm.. This is strange. I wonder when this changed. I definitely remember us changing our code to always lint anything explicitly passed on the command line. That was a long while ago, but that was the decision made.

@IanVS
Copy link
Member Author

IanVS commented Aug 25, 2016

I'd be interested to see that, because it must have been before my time (pre 1.0). I have never seen that behavior. It's always warned me when an explicitly passed file was ignored.

@nzakas
Copy link
Member

nzakas commented Aug 25, 2016

@IanVS You were part of the discussion: #3649 (comment)

@IanVS
Copy link
Member Author

IanVS commented Aug 25, 2016

@nzakas that was a slightly different issue which had to do with linting an explicitly-passed file with a non-.js extension. As you can see from the test which was added, it did not address ignored files at all.

@nzakas
Copy link
Member

nzakas commented Sep 8, 2016

Sorry, I lost track of this. :( Thanks for clarifying @IanVS, I see that I was conflating two issues.

In that case, I think this should be good to merge. @ilyavolodin thoughts?

@ilyavolodin
Copy link
Member

I think it's good to go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants