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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ff11eb
Enable Appveyor CI
erikkemperman May 8, 2017
4d955ca
Expect exclusively forward slash as path sep, same as node-glob
erikkemperman May 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ See:
* `man 3 fnmatch`
* `man 5 gitignore`

## Windows

**Please only use forward-slashes in glob expressions.**

Though windows uses either `/` or `\` as its path separator, only `/`
characters are used by this glob implementation. You must use
forward-slashes **only** in glob expressions. Back-slashes will always
be interpreted as escape characters, not path separators.

## Minimatch Class

Create a minimatch object by instantiating the `minimatch.Minimatch` class.
Expand Down
24 changes: 24 additions & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
environment:
matrix:
- nodejs_version: "7"
- nodejs_version: "6"
- nodejs_version: "4"

install:
- ps: Install-Product node $env:nodejs_version
- set CI=true
- npm -g install npm@latest
- set PATH=%APPDATA%\npm;%PATH%
- npm install

matrix:
fast_finish: true
build: off
version: '{build}'
shallow_clone: true
clone_depth: 1

test_script:
- node --version
- npm --version
- cmd: npm test
15 changes: 0 additions & 15 deletions minimatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
module.exports = minimatch
minimatch.Minimatch = Minimatch

var path = { sep: '/' }
try {
path = require('path')
} catch (er) {}

var GLOBSTAR = minimatch.GLOBSTAR = Minimatch.GLOBSTAR = {}
var expand = require('brace-expansion')

Expand Down Expand Up @@ -119,11 +114,6 @@ function Minimatch (pattern, options) {
if (!options) options = {}
pattern = pattern.trim()

// windows support: need to use /, not \
if (path.sep !== '/') {
pattern = pattern.split(path.sep).join('/')
}

this.options = options
this.set = []
this.pattern = pattern
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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

f = f.split(path.sep).join('/')
}

// treat the test path as a set of pathparts.
f = f.split(slashSplit)
this.debug(this.pattern, 'split', f)
Expand Down