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 some path / glob problems #4867

Merged
merged 7 commits into from Aug 31, 2020
Merged

Fix some path / glob problems #4867

merged 7 commits into from Aug 31, 2020

Conversation

@m-allanson
Copy link
Member

@m-allanson m-allanson commented Jul 16, 2020

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

#4521

Also:

It's an alternate take on this PR: #4863

Is there anything in the PR that needs further explanation?

The main behaviour change is:

  • if an input path contains globbing characters, it will be checked to see if it exactly matches an existing file.
  • if yes, the input path will be escaped so it gets treated as an exact path, instead of a glob.

This is broadly the approach used in ESLint, mentioned in #4388 (comment). It closes #4193.

This PR adds passing tests for most of the other problems reported in the above issues. I wasn't able to replicate some of the reported issues. They might be platform-specific, or may have been fixed in dependencies.

This doesn't fix #4855, but I'll add a comment on that issue instead.

@@ -118,6 +118,7 @@
"cosmiconfig": "^6.0.0",
"debug": "^4.1.1",
"execall": "^2.0.0",
"fast-glob": "^3.2.4",

This comment has been minimized.

@m-allanson

m-allanson Jul 16, 2020
Author Member

This is already a dependency of globby, so it's not really a new dependency here.

This comment has been minimized.

@evilebottnawi

evilebottnawi Jul 16, 2020
Member

Just note - prettier and many other tools remove globby and use fast-glob directly, maybe we can do same here

This comment has been minimized.

@m-allanson

m-allanson Jul 16, 2020
Author Member

Oh interesting! That could be a good follow-up PR.

It would need to be a breaking change, I think. As globbyOptions is a config option for stylelint. globby has a couple of options that aren't supported in fast-glob. Though I'd guess few people are using any of the globby-only option..

@m-allanson
Copy link
Member Author

@m-allanson m-allanson commented Jul 16, 2020

Some of the fixture paths are invalid on Windows. Will update.

`with spaces`,
`extglob!(s)`,
`got!negate/negate`,
// `extglob+(s)`, // Note: +'s cause errors. Ignoring until it becomes a problem

This comment has been minimized.

@malsf21

malsf21 Jul 17, 2020
Member

I'm curious, what's the error here? Should we document that there's a problem with the + character?

This comment has been minimized.

@m-allanson

m-allanson Jul 17, 2020
Author Member

Ah, I should have made a note of it. The + seemed to be escaped differently for this case, causing the expected match to fail.

I can't remember the specific details, but I think I tracked it down to somewhere in one of globby's dependencies. I thought I'd read an issue that described it, but now I can't find it 🤷

It seemed like enough of an edge-case that we can skip it for now, and investigate later if it becomes a problem.

@jeddy3
jeddy3 approved these changes Aug 1, 2020
Copy link
Member

@jeddy3 jeddy3 left a comment

Fantastic!

@jeddy3 jeddy3 mentioned this pull request Aug 9, 2020
6 of 6 tasks complete
@jeddy3 jeddy3 force-pushed the path-globs branch from d206c6d to 1ef46e0 Aug 31, 2020
@jeddy3 jeddy3 merged commit 181f3d9 into master Aug 31, 2020
11 checks passed
11 checks passed
CodeQL
Details
CodeQL
Details
Lint on Node.js 12 and ubuntu-latest
Details
Test on Node.js 10 and ubuntu-latest
Details
Test on Node.js 10 and windows-latest
Details
Test on Node.js 10 and macos-latest
Details
Test on Node.js 12 and windows-latest
Details
Test on Node.js 12 and macos-latest
Details
Coverage on Node.js 12 and ubuntu-latest
Details
CodeQL No new alerts
Details
coverage/coveralls Coverage increased (+0.003%) to 96.564%
Details
@jeddy3 jeddy3 deleted the path-globs branch Aug 31, 2020
m-allanson added a commit that referenced this pull request Sep 3, 2020
* master: (34 commits)
  Update CHANGELOG.md
  Fix double-slash disable comments when followed by another comment (#4913)
  Update CHANGELOG.md (#4916)
  13.7.0
  Prepare 13.7.0
  Prepare changelog
  Update dependencies
  Update CHANGELOG.md
  Deprecate *-blacklist/*-requirelist/*-whitelist (#4892)
  Fix some path / glob problems (#4867)
  Update CHANGELOG.md
  Add a reportDescriptionlessDisables flag (#4907)
  Fix CHANGELOG.md format via Prettier (#4910)
  Fix callbacks in tests (#4903)
  Update CHANGELOG.md
  Fix false positives for trailing combinator in selector-combinator-space-after (#4878)
  Add coc-stylelint (#4901)
  Update CHANGELOG.md
  Add support for *.cjs config files (#4905)
  Add a reportDisables secondary option (#4897)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.