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

Replace \ with / in paths on windows before passing to nanomatch #5808

Merged
merged 1 commit into from Feb 8, 2019

Conversation

Projects
4 participants
@dschep
Copy link
Member

dschep commented Feb 7, 2019

What did you implement:

fixes #5745 because nanomatch only supports / as a path separator

How did you implement it:

replace \ with / on windows before passing patterns into nanomatch

How can we verify it:

on windows:

npm i -g serverless/serverless#sls-5745
sls create -t aws-nodejs
sls plugin install -n serverless-offline
sls package

then if you check .serverless/aws-nodejs.zip, it should not contain the dev dependencies

Todos:

  • Write tests
  • n/a Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

replace \ with / in paths on windows before passing to nanomatch
fices #5745 becaue nanomatch only supports / as a path separator

@dschep dschep added this to In progress in Serverless via automation Feb 7, 2019

@dschep dschep requested a review from horike37 Feb 7, 2019

@pmuens pmuens added the pr/in-review label Feb 8, 2019

Serverless automation moved this from In progress to Reviewer approved Feb 8, 2019

@pmuens

pmuens approved these changes Feb 8, 2019

Copy link
Member

pmuens left a comment

Great fix and great "steps-to-reproduce" description 👌

Tested it on my Mac and it's still working (don't have a Windows machine handy).

Not sure if we should add a regression test here since we would mock anyways. Might be wortwhile to have smth. like that but since this bug is module-dependent and basically popped up because of "misuse" I'm fine with moving forward and merging w/o havin a regression test in place..

@pmuens pmuens changed the title replace \ with / in paths on windows before passing to nanomatch Replace \ with / in paths on windows before passing to nanomatch Feb 8, 2019

@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Feb 8, 2019

Yeah, i looked at the tests for that module to figure out how to test this, but came to the same conclusion you did.

@deltafactory

This comment has been minimized.

Copy link

deltafactory commented Feb 8, 2019

Are you calling Windows "misuse"? :)

@dschep dschep merged commit d21557a into master Feb 8, 2019

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

Serverless automation moved this from Reviewer approved to Done Feb 8, 2019

@dschep dschep deleted the sls-5745 branch Feb 8, 2019

@eahefnawy eahefnawy added this to the 1.38.0 milestone Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.