Fix: Always ignore defaults unless explicitly passed (fixes #5547) #5820

IanVS · 2016-04-11T03:47:54Z

I was getting some strange, seemingly unrelated failures on my machine (even on master) about valid-jsdoc. Hopefully it's just something wonky on my box.

eslintbot · 2016-04-11T03:47:58Z

LGTM

IanVS · 2016-04-11T03:52:19Z

Since Travis seems to be passing, this is the error I am getting (just recording here so I can investigate deeper at some later time):

  10386 passing (1m)
  2 failing

  1) valid-jsdoc valid /**
 * Foo
 * @module module-name
 */
function foo() {}:

      AssertionError: Should have no errors but had 1: [ { ruleId: 'valid-jsdoc',
    severity: 1,
    message: 'JSDoc syntax error.',
    line: 1,
    column: 1,
    nodeType: 'Block',
    source: '/**' } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:9447)
      at Context.<anonymous> (lib/testers/rule-tester.js:9:14196)

  2) valid-jsdoc valid /**
 * Foo
 * @alias module:module-name
 */
function foo() {}:

      AssertionError: Should have no errors but had 1: [ { ruleId: 'valid-jsdoc',
    severity: 1,
    message: 'JSDoc syntax error.',
    line: 1,
    column: 1,
    nodeType: 'Block',
    source: '/**' } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:9447)
      at Context.<anonymous> (lib/testers/rule-tester.js:9:14196)

alberto · 2016-04-11T13:21:57Z

Thanks @IanVS. I was in the process of putting a PR together to discuss with you about this, but didn't find the time.

I think right now ig.default contains all ignored patterns and ig.custom just the custom ones from .eslintignore. Do you think it makes sense to change it so that default contains just our defaults and custom (or all if you will) contains everything? So, we would use one or the other depending on whether we use --no-ignore

IanVS · 2016-04-11T13:36:12Z

Hm, that's an interesting thought, and it might help improve the clarity of the code. I guess my suggestion would be to merge this if it looks okay to the reviewers, and then we can use it and its tests as a baseline to refactor as you suggest.

platinumazure · 2016-04-12T14:17:50Z

I like @alberto's suggestion and @IanVS's implementation plan. Anything else holding this back?

IanVS · 2016-04-12T14:21:01Z

Just needs a review from a reviewer.

Hm, there's no @ mention for "reviewers-team".

alberto · 2016-04-14T09:04:23Z

tests/lib/util/glob-util.js

+            assert.equal(result.length, 0);
+        });
+
+        it("should ignore and warn for default ignored files when passed explicitly", function() {


@nzakas Is this the expected behavior? I understood otherwise from these comments

I think that's the big we are fixing here, no?

I think we may be getting mixed up on a few related issues. This PR is to fix #5547, which is to say, to keep ignoring default-ignored files even if the --no-ignore flag is passed.

The current behavior has always been to print a warning if an explicitly passed file is ignored due to an ignore rule. #4828 (comment) is a separate issue and will need greater structural changes to accomplish.

I'm not sure we should add a test for a behaviour we think is a bug, but yeah, it's a different issue.

nzakas · 2016-04-23T15:45:09Z

@IanVS are you still working on this?

IanVS · 2016-04-25T02:25:33Z

I'm unsure what the required tasks are, to be honest. Do I need to create a different warning message in cases where it's necessary to use a special override pattern to unignore a default ignore? It seems like it may be short lived if we stop using such warnings at all, as seems to be the direction we've decided to go.

alberto · 2016-04-25T20:51:34Z

@IanVS I'm not sure when that would be done (we don't even have an specific issue for it and I'm not 100% sure it's without problems), so I'd say it's better to add it for now.

nzakas · 2016-04-26T00:25:18Z

@alberto is this ready to merge in your opinion?

I've gotten a bit lost here, so don't want to assume anything. :)

IanVS · 2016-04-26T00:44:47Z

I need to make a small update to the warning messages. Working on it now.

eslintbot · 2016-04-26T01:37:00Z

LGTM

IanVS · 2016-04-26T01:43:23Z

I just now signed the jQuery CLA.

@alberto once you give the 👍 to my latest commit, I'll squash commits. Wanted to make it easier to see the new changes.

alberto · 2016-04-26T07:11:46Z

lib/cli-engine.js

+    var message;
+
+    if (/^\./.test(path.basename(filePath))) {
+        message = "Hidden file ignored by default.  Use '--ignore-pattern !.*' to override.";


EDITED: Correcting myself inline to avoid adding more confusion, sorry.

I think it would be better not to recommend to unignore everything, but just the current file in this case.

Also the ignore patterns have to be quoted in all cases. e.g. Use '--ignore-pattern \"!node_modules/*\"' to override.

I was trying to keep this as simple as possible, knowing that the fancier I make it the more likely there will be bugs. It's already pretty ugly, and hopefully can be cleaned up when we refactor to simplify default ignores. For now, would you be opposed to a simpler message like:

File ignored by default. Use a negated --ignore-pattern (like '!<filename>') to override."

That's what I was proposing initially. Unfortunately it won't work for files in nested folders in node_modules or bower_components. You can't just unignore node_modules/foo/bar.js, you have to unignore node_modules/foo/ :/

I think you misunderstand. I mean that message literally. Not using a variable. Let the user figure out what the pattern should be, after giving them the hint to use a !.

To be clear, I'm proposing to adapt it like this:

var relativePath = path.relative(baseDir, filePath)); if (/^\./.test(path.basename(filePath))) { message = "File ignored by default. Use '--ignore-pattern \"!" + relativePath + "\"' to override."; } else if (baseDir && /^node_modules/.test(relativePath) { message = "File ignored by default. Use '--ignore-pattern \"!node_modules/*\"' to override."; } else if (baseDir && /^bower_components/.test(path.relative(baseDir, filePath))) { message = "File ignored by default. Use '--ignore-pattern \"!bower_components/*\"' to override."; } else { message = "File ignored because of a matching ignore pattern. Use '--no-ignore' to override."; }

I did, but to be able to figure out the right pattern yourself (as I said, you can't simply unignore the file and I think that's what people would try given that message), you'd have to know we are using node_modules/* to ignore.

Yes, but I would like to avoid using relativePath directly in the message because baseDir may not always be provided and there may be other edge cases and conditions that cause trouble. I'd rather replace all of that logic with:

var isHidden = /^\./.test(path.basename(filePath)); var isInNodeModules = baseDir && /^node_modules/.test(path.relative(baseDir, filePath)); var isInBowerComponents = baseDir && /^bower_components/.test(path.relative(baseDir, filePath)); if (isHidden || isInNodeModules || isInBowerComponents) { message = "File ignored by default. Use a negated --ignore-pattern (like '!<relative/path/to/filename>') to override."; } else { message = "File ignored because of a matching ignore pattern. Use '--no-ignore' to override."; }

This is still more complicated than I'd like and still relies on baseDir for node_modules and bower_components (no way around that), but does not require it for hidden files. The more explicit we are in the error message (by giving an exact flag to use), the greater chances for confusion if/when that suggestion does not work.

Are you sure !node_modules/foo/bar.js won't work? That would be annoying. . .

You can try it on your branch with:

eslint node_modules/escope/gulpfile.js --ignore-pattern '!node_modules/escope/gulpfile.js'

vs:

eslint node_modules/escope/gulpfile.js --ignore-pattern '!node_modules/*'

Also, bash needs single, not double quotes with "!" for some reason. :(

alberto · 2016-04-26T13:51:52Z

@IanVS If we change our default ignore patterns to:

"/node_modules/**/*.js",
"/bower_components/**/*.js"

This should allow directly unignoring individual files. :)

IanVS · 2016-04-26T14:03:04Z

Yes, but I would be worried about other unintended consequences of doing that. For example if someone uses --ext .es6 and there happen to be .es6 files in a node module.

I think that leaves us with the following as the best-case we can hope for at this time. Do you agree?

var isHidden = /^\./.test(path.basename(filePath));
var isInNodeModules = baseDir && /^node_modules/.test(path.relative(baseDir, filePath));
var isInBowerComponents = baseDir && /^bower_components/.test(path.relative(baseDir, filePath));

if (isHidden) {
  message = "File ignored by default.  Use a negated --ignore-pattern (like '!<relative/path/to/filename>') to override.";
} else if (isInNodeModules) {
  message = "File ignored by default. Use '--ignore-pattern \"!node_modules/*\"' to override.";
} else if (isInBowerComponents) {
  message = "File ignored by default. Use '--ignore-pattern \"!bower_components/*\"' to override.";
} else {
  message = "File ignored because of a matching ignore pattern. Use '--no-ignore' to override.";
}

alberto · 2016-04-26T15:31:51Z

[...] I would like to avoid using relativePath directly in the message because baseDir may not always be provided and there may be other edge cases and conditions that cause trouble.

I would expect this would work the same as ignored-paths, which uses process.cwd() by default, since this is directly related. But if you still think it's better not to do it, it's not a big deal.

Yes, but I would be worried about other unintended consequences of doing that. For example if someone uses --ext .es6 and there happen to be .es6 files in a node module.

Ok, let's leave that out of this issue.

Do you agree?

Sorry, I was wrong. You need to use single quotes instead of double quotes around the !node_modules/* and !bower_components/*patterns, otherwise bash tries to expand the !

eslintbot · 2016-04-28T12:50:09Z

LGTM

IanVS · 2016-04-28T12:52:43Z

OK @alberto, I've updated the wording as we've discussed. Anything else you see that I should take care of? Thanks for working through this together.

nzakas · 2016-04-28T16:02:51Z

Since we are doing a release tomorrow and it looks like there might be some outstanding ignore issues (#5979), let's hold off on merging this at least until Tuesday (to be clear of any patch release), and then we can have two weeks of baking on master to be sure of the changes.

alberto · 2016-04-28T16:25:35Z

LGTM. 👍

Thanks @IanVS for keeping up with this.

kaelzhang · 2016-04-29T03:53:20Z

Sorry for the ignore issues I brought it to you guys, and for my delay of fixing these issues, that there is a time lag between us, and I could only check issues and make the fixing while you are asleep.

However, all confirmed bugs of node-ignore will always be fixed within 24 hours. You have my word.

alberto · 2016-04-29T06:06:11Z

Thanks @kaelzhang, no need to apologize for bugs, they are unavoidable.

And we are also spread over the world, have our jobs, time of sleep, have our own lives and all, so we don't expect a SLA from anyone. :)

Thanks for your work. ❤️

eslintbot added the cla found label Apr 11, 2016

IanVS mentioned this pull request Apr 11, 2016

Can't pass dot/hidden files on command line #4828

Closed

alberto reviewed Apr 14, 2016
View reviewed changes

IanVS added 2 commits April 25, 2016 21:28

Fix: Always ignore defaults unless explicitly passed (fixes #5547)

0e13b3c

Base ignore message on type of ignore

f8308f2

IanVS force-pushed the issue-5547 branch from af84ea6 to f8308f2 Compare April 26, 2016 01:36

jquerybot added CLA: Error and removed CLA: Error labels Apr 26, 2016

alberto reviewed Apr 26, 2016
View reviewed changes

Tweak ignore warning messages

d829c92

nzakas removed the cla found label Apr 29, 2016

nzakas merged commit eb2fb44 into master May 2, 2016

IanVS deleted the issue-5547 branch May 3, 2016 04:04

Arcanemagus mentioned this pull request Jun 9, 2016

eslint@2.11.1 AtomLinter/linter-eslint#593

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Fix: Always ignore defaults unless explicitly passed (fixes #5547) #5820

Fix: Always ignore defaults unless explicitly passed (fixes #5547) #5820

Conversation

IanVS commented Apr 11, 2016

eslintbot commented Apr 11, 2016

IanVS commented Apr 11, 2016

alberto commented Apr 11, 2016

IanVS commented Apr 11, 2016

platinumazure commented Apr 12, 2016

IanVS commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Apr 23, 2016

IanVS commented Apr 25, 2016

alberto commented Apr 25, 2016

nzakas commented Apr 26, 2016

IanVS commented Apr 26, 2016

eslintbot commented Apr 26, 2016

IanVS commented Apr 26, 2016

alberto Apr 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS Apr 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alberto commented Apr 26, 2016

IanVS commented Apr 26, 2016

alberto commented Apr 26, 2016

eslintbot commented Apr 28, 2016

IanVS commented Apr 28, 2016

nzakas commented Apr 28, 2016 • edited

alberto commented Apr 28, 2016

kaelzhang commented Apr 29, 2016

alberto commented Apr 29, 2016

alberto Apr 26, 2016 •

edited

IanVS Apr 26, 2016 •

edited

nzakas commented Apr 28, 2016 •

edited