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

fix(hook): handle node module path os agnostically #114

Merged
merged 2 commits into from
Jul 4, 2016

Conversation

ta2edchimp
Copy link
Collaborator

Former hook name determination was a simple regex using *nix style path separator /, using path.basename, the required last part gets returned, independent of an os specific path separator.

Former `hook` name determination was a simple regex using *nix style path separator `/`, using `path.basename`, the required last part gets returned, independent of an os specific path separator.
@codecov-io
Copy link

codecov-io commented Jul 4, 2016

Current coverage is 100%

Merging #114 into master will not change coverage

@@           master   #114   diff @@
====================================
  Files           4      4          
  Lines         121    118     -3   
  Methods         0      0          
  Messages        0      0          
  Branches        4      4          
====================================
- Hits          121    118     -3   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last updated by 32edf9c...067311c

@ta2edchimp ta2edchimp changed the title fix(hook): detemine hook name path separator agnostically fix(hook): handle node module path os agnostically Jul 4, 2016
@ta2edchimp
Copy link
Collaborator Author

ta2edchimp commented Jul 4, 2016

Should we investigate means to also automatically test on Windows environments? (AppVeyor comes to my mind, but atm I have no clue how we could get the semantic releases to also depend on successful builds on another service ...)

@kentcdodds
Copy link
Contributor

Yes, this is needed.

@kentcdodds kentcdodds merged commit 26b5fa7 into ghooks-org:master Jul 4, 2016
@kentcdodds
Copy link
Contributor

Should we investigate means to also automatically test on Windows environments?

Yeah, probably should

(AppVeyor comes to my mind, but atm I have no clue how we could get the semantic releases to also depend on successful builds on another service ...)

We could create a release branch and merge master into that branch when we're ready to do the release. Requires an extra step, but that would allow us to make sure that things look good.

Or we could just keep things as-is and rely on the pull request status checks. If the PR passes AppVeyor, then we'll assume that it'll pass when it merges into master. I think that'd be sufficient.

I've tried AppVeyor once and failed to get it working. But I think it'd be great to have it working on this project.

@ta2edchimp
Copy link
Collaborator Author

Or we could just keep things as-is and rely on the pull request status checks. If the PR passes AppVeyor, then we'll assume that it'll pass when it merges into master. I think that'd be sufficient.

Sounds like a plan that should suffice.

@gtramontina
Copy link
Collaborator

I agree with this second approach as it seems simpler to get working. If I remember correctly, we can set github up so it doesn't allow for merging PRs if some checks fail…

BTW, thanks for addressing this issue right away!!

@gtramontina gtramontina mentioned this pull request Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants