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

Ignoring improvements #2464

Merged
merged 8 commits into from Apr 22, 2017

Conversation

5 participants
@davidtheclark
Contributor

davidtheclark commented Apr 1, 2017

The aim is to close #2399, close #2236 , and replace PR #2382.

  • As part of this, I've decided to remove the allowEmptyInput. I think that the opinion has come up before that we could just silently not lint any files, if no files match. This is what ESLint does, and I'm happy to stick with that. The reason I'm doing it here is that remove that option removes some complexity we would otherwise face with the new ignoring system.
  • Changes syntax of .stylelintignore from strict .gitignore globs to node-glob globs. I think this won't bother anybody, because it seems like the .gitignore patterns are a subset of the node-glob patterns — and I preserved the ability to add comments.

There's more work to be done, especially in finding code to delete and tests to clarify.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 1, 2017

It's looking great so far!

As part of this, I've decided to remove the allowEmptyInput

SGTM.

Changes syntax of .stylelintignore from strict .gitignore globs to node-glob globs

👍

@davidtheclark davidtheclark force-pushed the improve-ignore branch from f885acb to eb7dc58 Apr 1, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 1, 2017

Added disableDefaultIgnores option to standalone and CLI.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 1, 2017

I think this does the trick now.

@ArmorDarks before we merge this change, can you help us out by trying this branch? I ask because you filed the "slow globbing" issue and have a codebase large enough to notice the difference if we've fixed the situation.

What you'll need to do to test this solution is:

  • Move your ignoreFiles globs to .stylelintignore. (ignoreFiles is always going to be inefficient; but .stylelintignore should be efficient — we'll recommend this in the docs.)
  • Run stylelint out of this branch (node lib/cli.js ...) and see if it's faster than before in your codebase.

Let me know what you find!

davidtheclark added some commits Apr 1, 2017

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Apr 1, 2017

Hi

Sorry for the delay. Sure, I will try to do it within few days, if it's ok.

@davidtheclark davidtheclark changed the title from [WIP] Ignoring improvements to Ignoring improvements Apr 1, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 2, 2017

I think this does the trick now.

Nice! The code looks great.

I'm happy a bit of trouble making this branch work, though. I have this folder structure:

/src/
  /1/
    test.css
  /2/
    test.css
.gitignore
.stylelintignore
.stylelintrc.json

With the content of .gitignore and .stylelintignore being:

src/1

Am I right in thinking that both these should ignore the tests.css file in 1?:

node ../stylelint/lib/cli.js "src/**/*.css"
node ../stylelint/lib/cli.js "src/**/*.css" --ignore-file .gitignore

Neither does, though. I've tried different ignore globs too e.g. src/1/*.css

I'd like to quickly determine if there's something amiss with my local setup. It seems likely as the tests in the PR look sound. As such, can someone else in @stylelint/core try this branch against their codebase and post their findings.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 2, 2017

🤔 The problem seems to be in the way that globby deals with relative and absolute paths. Duplicating your setup, here's the globs I send to globby, and the files I get back:

[ 'src/1/test.css',
  'src/2/test.css',
  '!**/node_modules/**',
  '!**/bower_components/**',
  '!/Users/davidclark/Downloads/test/src/1' ]
[ 'src/1/test.css', 'src/2/test.css' ]

If, however, I specify absolute paths for the input, here's what I get (it works):

[ '/Users/davidtheclark/Downloads/test/src/**/*.css',
  '!**/node_modules/**',
  '!**/bower_components/**',
  '!/Users/davidclark/Downloads/test/src/1' ]
[]

I'll look into whether this is expected behavior or not. We could probably fix it if necessary by making the input files absolute ...

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 2, 2017

Ok, so part of this looks to be solved (if tests still passed) by not absolutizing the paths we read from the ignore file.

However, another point came up when trying this: I was too hopeful that .gitignore would fit nicely into glob syntax. It won't. src/1 will work in .gitignore but not with a glob: you need src/1/**/* or something like that.

The trouble here is that we really don't want to analyze things twice, with two separate interpreters. We want to pass globby the right glob, not one without ignored files which we then filter.

We could try to get around this trouble by adjusting for directories:

  • For any line ending with a slash, add **/* (e.g. src/1/).
  • For any line not ending with an extension, add **/* (e.g. src/1).

I'm not sure if that would cover all the bases. Anybody want to investigate? (I may or may not have time to do so today.)

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 2, 2017

Tests did not pass, so looks like no parts are solved 😞

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 2, 2017

I think I got it ...

Latest commit restores the specific .gitignore syntax, because that's preferable to regular globs for this situation. It uses ignore's filter after the glob is analyzed but before any files are read. Glob-reading is pretty efficient, I think — I just read ~16000 files and it took under a second. So the inefficiency is when we load the file's config before determining whether it's ignored or not. This setup avoids that.

@jeddy3: I believe this works in the test-case you offered now. Want to double-check?

@davidtheclark davidtheclark force-pushed the improve-ignore branch from 831caf5 to 32cfedd Apr 2, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 3, 2017

I believe this works in the test-case you offered now. Want to double-check?

Worked a treat thanks! I was able to test .stylelintignore, --ignore-path and --disable-default-ignores against my unwieldy stylelint test folder that has files all over the place. The branch appear to work great with the various ignore pattern I threw at it.

I just read ~16000 files and it took under a second. So the inefficiency is when we load the file's config before determining whether it's ignored or not. This setup avoids that.

That's excellent news.

@jeddy3

jeddy3 approved these changes Apr 3, 2017

LGTM

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 7, 2017

@stylelint/core Anyone else use ignores in the projects? If so, could you possibly give this branch a whirl?

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 22, 2017

Let's merge this now, so the PR does not grow stale; and if anybody has problems or more concerns they can create new issues.

@davidtheclark davidtheclark merged commit ea33c17 into v8 Apr 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@davidtheclark davidtheclark deleted the improve-ignore branch Apr 22, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 22, 2017

Added to changelog:

  • Added: disableDefaultIgnores option (--disable-default-ignores in CLI), to allow linting of node_modules and bower_components directories (#2464).
  • Added: more efficient file ignoring with .stylelintignore (#2464).
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 22, 2017

To be clear: added those to the v8 changelog.

@zhaozhiming zhaozhiming referenced this pull request May 12, 2017

Closed

Add stylelint #1216

jeddy3 added a commit that referenced this pull request Jul 14, 2017

8.0.0 (#2735)
* Add stylelint semantic versioning policy

* Fix whitespace and empty lines

* Add changelog entry

* Remove stylelint-config=standard reference

* Tweak major release optional options verbiage

* Update changelog

* Props ESLint

* Fix false negatives with ignore: ["descendant"] in selector-no-type (#2200)

* Update CHANGELOG.md

* Fix handling of shared-line comments (#2262)

* Fix handling of shared-line comments

Introduces a number of utilities and adjusts the following rules:
at-rule-empty-line-before, custom-property-empty-line-before,
declaration-empty-line-before, rule-nested-empty-line-before,
rule-non-nested-empty-line-before

Closes #2237; closes #2239; closes #2240.

* Fixes based on feedback

* Use getPreviousNonSharedLineCommentNode more

* Use more new helpers in comment-empty-line-before

* Add ignore: ["child"] option to selector-no-type (#2217)

* Add ignore: ["child"] option to selector-no-type

* Correct logic for ignore: ["child"] option

* Include line and column in reject tests with multiple type selectors

* Update CHANGELOG.md

* Report every selector resolved selector in selector-max-compound-selectors (#2350)

* Update CHANGELOG.md

* Add new shorthand data (#2354)

* Add new shorthand data

* Add mask shorthand

* Use defined list of properties in shorthand-property-no-redundant-values

* Improve declaration-block-no-shorthand-property-overrides speed a little bit

* Check prefixed properties agains prefixed properties in declaration-block-no-shorthand-property-overrides

* Improve declaration-block-no-redundant-longhand-properties speed a little bit

* Ignore prefixes case in declaration-block-no-shorthand-property-overrides

* Check prefixed properties agains prefixed properties in declaration-block-no-redundant-longhand-properties

* Add `grid-gap` to shorthand-property-no-redundant-values

* Explain properties set for shorthand-property-no-redundant-values

* Revert unnecessary changes

* Add “mask” to declaration-block-no-redundant-longhand-properties readme

* Update CHANGELOG.md

* Update changelog

* Remove deprecated rules (#2422)

* Remove deprecated rules

* Include `rule-empty-line-before` in system tests

* *-empty-line-before rules consider line as empty if it contains whitespace only (#2440)

* Update CHANGELOG.md

* Remove deprecated options (#2433)

* Update CHANGELOG.md

* report-needless-disables now exits with non-zero code (#2341)

* Update CHANGELOG.md

* Remove leftover files (#2442)

* Remove leftover file

* One more

* And one more

* Ignoring improvements (#2464)

* WIP ignoring improvements

* Remove standaloneIgnored property of result

* Add disableDefaultIgnores

* Add documentation tweaks

* Add parseStylelintIgnore tests

* Non-absolutized ignore file paths

* Restire gitignore syntax

* Remove parseStylelintIgnore

* Update CHANGELOG.md

* Check all linear-gradients in a value list (#2496)

Closes #2481

* Update CHANGELOG.md

* Update CHANGELOG.md

* Respect line case for `ignorePattern` (#2683)

Closes #2647

* Update CHANGELOG.md

* Update decls

* Fix remark warning

* Update snapshots

* Add missing message to reject test

* Fix tests for *-empty-line-before rules (#2688)

* Fix tests for *-empty-line-before rules

* Test addition and tweak

* Update to PostCSS@6 (#2561)

* Update to PostCSS@6

* Fix PostCSS@6 closing-brace test (#2689)

* Account for PostCSS@6's support for at-apply

* Remove only

* Fix postcssPlugin tests (#2690)

* Fix defaultSeverity.test.js test

* Fix ignoreDisables test

* Check ownSemiColon

* Fixed: remove hacks related to custom property set.

* Remove deprecated rules (#2693)

Closes #2521

* Update CHANGELOG.md

* Update CHANGELOG.md

* Add VISION.md (#2704)

* Add releases.md (#2705)

* Add releases.md

Closes #2700

* Add dry-release step

* Use HTTPS for links

* Change ignore options for selector-max-type (#2701)

* Fix false negatives with ignore descendant

* Add ignore child

Closes #2699

* Update CHANGELOG.md

* Update README.md to account for VISION.md (#2727)

* Update README.md to account for VISION.md

* Fix lint warnings in CHANGELOG

* Update postcss-less to 1.0 (#2702)

* Update postcss-less to 1.0.2

* Update isStandardSyntaxRule for postcss-less 1.0

* Pass custom postcss stringifier when creating result

* Correct no-extra-semicolons Less test position

* Ignore Less mixins in no-extra-semicolons

* Update postcss-less to 1.1.0

* Correct no-extra-semicolons Less test position again

* Utilize isStandardSyntaxRule when checking for Less rules in no-extra-semicolons

* Case sensitive white/blacklists and ignore* options (#2709)

Closes #2660

* Update CHANGELOG.md

* docs: Add "New Release Issue Template" to releases.md (#2728)

* Exit with non-zero code on postcss warnings (#2713)

* Enhancement: exit with non zero code on postcss warnings.

* Account for parseErrors in stringformatqer

* Guard against no parseErrors

* Update CHANGELOG.md

* Refactor: simplify ignore logic. (#2732)

* Update changelog

* Bump 8.0.0 changes to top

* Correctly place changelog items

* Add introduction

* Add codefence for website

* Use relative path

* Include VISION in package for website

* Add link to VISION doc for website

* Changelog amends

* Update releases.md (#2738)

* Update eslint and config (#2737)

* Order requires

* Use eqeqeq eslint rule

* Update eslint

* Disable no-useless-escape just for this repo

* Enable no-useless-escape rule

* Use tilde

* Fix enable comment

* Add reject test for unescaped regex
@alexgorbatchev

This comment has been minimized.

alexgorbatchev commented Jun 19, 2018

we could just silently not lint any files, if no files match. This is what ESLint does, and I'm happy to stick with that.

I'm writing a CLI tool that executed stylelint. I found it problematic to test integration because i can't distinguish between the case where I'm passing bad arguments on purpose and the case where file validates OK. Essentially, I'm only able to test if the file has linting issues.

Furthermore, I think if specific file path is passed and file doesn't exist, it should fail. I'm asking for a file to be linted, it's not there, therefore something is wrong. If the glob is passed, current behavior feels ok.

What do you think?

@ntwb

This comment has been minimized.

Member

ntwb commented Jun 20, 2018

@alexgorbatchev Thanks for the comment, would you mind creating a new issue to discuss this please :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment