Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: 🐛 normalize path before check is glob #337

Closed
wants to merge 7 commits into from
Closed

fix: 🐛 normalize path before check is glob #337

wants to merge 7 commits into from

Conversation

VdustR
Copy link

@VdustR VdustR commented Feb 18, 2019

normalize the path for wildcard in windows

Issues: fix #317

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

fix #317

Breaking Changes

n/a

Additional Info

fixed in the same way with egoist/poi#533

@jsf-clabot
Copy link

jsf-clabot commented Feb 18, 2019

CLA assistant check
All committers have signed the CLA.

"p-limit": "^2.1.0"
"normalize-path": "^3.0.0",
"p-limit": "^2.1.0",
"serialize-javascript": "^1.4.0"
},
"devDependencies": {
"@babel/cli": "^7.1.5",
Copy link
Member

Choose a reason for hiding this comment

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

Don't touch this file in this PR

Copy link
Author

Choose a reason for hiding this comment

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

is it okay that eslint warning import/no-extraneous-dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

@VdustR on what?

Copy link
Author

Choose a reason for hiding this comment

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

import normalizePath from 'normalize-path';

Copy link
Member

Choose a reason for hiding this comment

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

Oh, all good, my mistake, tired

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, let's add test

@codecov
Copy link

codecov bot commented Feb 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a1c5372). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #337   +/-   ##
========================================
  Coverage          ?   95.4%           
========================================
  Files             ?       9           
  Lines             ?     283           
  Branches          ?      77           
========================================
  Hits              ?     270           
  Misses            ?      12           
  Partials          ?       1
Impacted Files Coverage Δ
src/preProcessPattern.js 95.89% <100%> (ø)
src/utils/normalizePatternFrom.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1c5372...8093628. Read the comment docs.

normalize the path for wildcard in windows

Issues: fix #317
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexander-akait
Copy link
Member

Please accept CLA

@VdustR
Copy link
Author

VdustR commented Feb 18, 2019

Please accept CLA

signed

...pattern.from,
glob: normalizePath(pattern.from.glob),
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Find one interesting thing https://github.com/floatdrop/gulp-watch/blob/master/index.js#L54 , we need add test for '!'

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I didn't get it.

Do you mean test '[!]\\hello.txt'?

Copy link
Member

@alexander-akait alexander-akait Feb 18, 2019

Choose a reason for hiding this comment

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

Something like this !(directory)/**/*.txt (and !(directory)\**\*.txt)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We should add this logic to https://github.com/webpack-contrib/copy-webpack-plugin/blob/master/src/utils/escape.js, because not only glob (file and dir can also) type can contains / and \, we should normalize them too, anyway thanks for PR, i will do new PR and put normalize in right place and with you tests and code

@VdustR VdustR deleted the fix/is-glob/windows branch February 19, 2019 19:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isGlob return false for windows path with backslashes
4 participants