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
Conversation
LGTM |
LGTM |
// regex to find .hidden or /.hidden patterns, but not ./relative or ../relative | ||
const globIncludesDotfiles = /(?:(?:^\.)|(?:[\/\\]\.))[^\/\\\.].*/.test(pattern); | ||
|
||
options.dotfiles = options.dotfiles || globIncludesDotfiles; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- specifies a glob which would include dotfiles (e.g.
.*
ortest/**/.*
) (this is what the regex I added is intended to determine) - unignores dotfiles with an
--ignore-pattern
or pattern in an.eslintignore
file. This is similar to our other default ignores ofnode_modules
andbower_components
There was a problem hiding this comment.
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.
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:
|
Will do. 👍 On Thu, Aug 11, 2016, 12:57 Nicholas C. Zakas notifications@github.com
|
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 "test:lint": "eslint . .storybook", Thanks again for your excellent work on ESLint! |
@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.
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
No, again that is unchanged here. (although see the answer to
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 If, however, the shell does not expand, or the user adds double quotes around the glob (
This is the other change in this PR. As I said above, 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. |
@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: Is that correct? |
@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 |
@IanVS Yes, a glob like the one you provided. And I'm asking about this PR. |
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 |
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:
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. |
BTW, I will rebase this with master tonight. |
LGTM |
I've rebased onto master. |
@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. |
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. |
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. |
@IanVS You were part of the discussion: #3649 (comment) |
@nzakas that was a slightly different issue which had to do with linting an explicitly-passed file with a non- |
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? |
I think it's good to go. |
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 tolistFilesToProcess
, 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.