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

Extract local path logic and add tests. #341

Merged
merged 15 commits into from
Feb 11, 2017
Merged

Conversation

zigomir
Copy link
Contributor

@zigomir zigomir commented Feb 9, 2017

Fixes #300 and #283.

Thanks @ajanes93 for opening a PR. Also thanks to @likun7981 for providing regex.
Only thing wrong in regex imho was assumption that windows drive letter can be of any length and thus bitbucket:username/rep was treated as local template. I fixed this in this PR and also added some tests.

I'm aware that running these tests on Windows won't pass because of isAbsolute used, but CircleCi runes on *nix so... I'm open to any suggestions.

@egoist what do you think?

@posva
Copy link
Member

posva commented Feb 9, 2017

Maybe we should use appveyor for vue-cli to run tests on a windows machine too

@zigomir
Copy link
Contributor Author

zigomir commented Feb 10, 2017

@posva I added appveyor which is finally passing now. I had configuration wrong and it was running against node 4 (which is fine) but there was npm v2 installed which wasn't fine.

I also found we had some race conditions in tests, reading from same directory (thanks Windows) etc. Should be all fixed in this PR now.

@posva
Copy link
Member

posva commented Feb 11, 2017

@zigomir 😆 I see that you had some trouble due to the commit history. I hope it wasn't too much of a pain


module.exports = {
isLocalPath: function (templatePath) {
return /^[./]|(^[a-z|A-Z]:)/.test(templatePath)
Copy link
Member

@posva posva Feb 11, 2017

Choose a reason for hiding this comment

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

Why is the | inside of the [] necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, might not be at all :) (regex by trial and error image here 😊)

- CR feedback
@zigomir
Copy link
Contributor Author

zigomir commented Feb 11, 2017

@posva hehe yeah. No worries, I think it's good that we run tests on Windows too. + I learned a bit how appveyor works.

I changed regex as you pointed out in CR. Let me know if there is anything else we should change.

Also, worth noting are weird race condition issues that were resolved here: 1f15a01

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me 🙂

@Qquanwei
Copy link

thank your work <3
I need this feature , but it seems not publish to npm, what should I do?

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.

3 participants