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

Proposal: major release to adopt forward slash, same as node-glob? #109

Closed
wants to merge 2 commits into from

Conversation

erikkemperman
Copy link
Contributor

As folks have pointed out before, minimatch misbehaves somewhat on Windows -- and it ends up sabotaging the documented usage of node-glob, i.e. to "Please only use forward-slashes in glob expressions".

In the spirit of constructive feedback, I figured I'd submit a PR. The first commit adds Appveyor CI configuration, and causes a bunch of failed tests.

The second removes the substitution of platform-specific path separator -- the assumption being that the caller follows the convention of only using forward-slashes.

Needless to say this would be a breaking change.

Related:
#92
#78
#64
#103
isaacs/node-glob#212

@erikkemperman erikkemperman changed the title Proposal: major release with breaking change to forward slash Proposal: major release to adopt forward slash, same as node-glob? May 8, 2017
@erikkemperman erikkemperman force-pushed the windows-fix branch 2 times, most recently from 0ab1d06 to 63eb5ad Compare May 8, 2017 15:02
@erikkemperman erikkemperman deleted the windows-fix branch May 8, 2017 15:11
@erikkemperman erikkemperman restored the windows-fix branch May 8, 2017 15:11
@erikkemperman
Copy link
Contributor Author

Aargh, apologies, closed the wrong PR. Reopening.

@@ -708,11 +698,6 @@ function match (f, partial) {

var options = this.options

// windows: need to use /, not \
if (path.sep !== '/') {
Copy link
Contributor

@litmit litmit Jun 21, 2017

Choose a reason for hiding this comment

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

It's wrong. On Windows '/' character in pattern should match both '/' and '\' characters in file paths. There is not ideal slow implementation (i think this should be done in matching engine) but it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. The idea here is to have a cross-platform API, with forward slash as path separator and backward slash as escape. That way you can still match literal forward (on Windows) and backward slashes by escaping them in the pattern.

Copy link
Contributor

@litmit litmit Jun 21, 2017

Choose a reason for hiding this comment

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

Pattern (as some kind of special language) SHOULD be cross-platform, and therefore need to use one special character (forward slash) as path separator on all platforms.
But, in opposite, file path is platform depended by definition .
For example, code:
if ( minimatch(filepath, 'foo/*.js') ) { /* do something*/ }
will perfectly works with current minimatch version (or with my fork) on all platforms.
But with your commit the condition will do never trigger on Windows.

Moreover, imagine that one day will be invented a new operation system, and it developers decide to use pipe characted ('|') as path separator. The code example will work on this OS without any changes, because filepath foo|bar.js will be internally replaced with foo/bar.js (using code that you try to remove) and successfully match to the pattern foo/*.js

@litmit
Copy link
Contributor

litmit commented Jun 21, 2017

See also https://github.com/litmit/minimatch/tree/match-for-windows (works on Windows just enough for me).
But I'm converting only slashes in paths not patterns.

@AmeliaBR
Copy link

Has this proposal been superseded by micromatch/nanomatch@4d25ac8, or is it still being considered?

Has there been any discussion about making this a configurable option, with three possible values:

  • allow only '/' as a separator in paths and patterns, a \ is treated as a regular character (Strict behavior)
  • allow either / or \ to be a separator, and treat them as interchangeable when matching (Windows behavior)
  • match the current platform default (probably a sensible default, approximating current behavior, but more consistently)

FYI, I'm using Metalsmith, which uses minimatch heavily in the core library and its plugin ecosystem. On Windows systems, they do not normalize paths at all, instead relying on the Node core Path methods, which use platform-dependent separators.

But of course, I want my code to also work when a colleague runs it on a Unix system, so my patterns all use / as a separator. Right now (minimatch 3.0.4), I find that regular matching works, but the Regex returned by makeRe don't.

@litmit
Copy link
Contributor

litmit commented Mar 29, 2018

@AmeliaBR there is should be one valid option :)

  • allow only '/' as a separator in patterns, and any separators that allowed by the platform in the file paths.

@AmeliaBR
Copy link

@litmit

On reflection, I do agree that only / should be allowed in patterns, to avoid ambiguity.

But I'd still like the option of controlling what this matches to, depending on whether I'm doing path normalization before calling the library.

@litmit
Copy link
Contributor

litmit commented Mar 30, 2018

@AmeliaBR
If / match to any valid platfom specific path separator then you need no care on whether you are doing path normalization before calling the library. It will match in any case.

The option that you suggested is needed, IMHO, in two rare cases:

  • when the glob matching library used to match anything aside from the filepaths;
  • when need to match to the flile path that came from a different OS.

@isaacs
Copy link
Owner

isaacs commented Feb 15, 2022

Done on v5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants