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

Small fixes for contribution guide #616

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

foxxyz
Copy link
Contributor

@foxxyz foxxyz commented Aug 23, 2019

While following the contribution guide, I noticed a few small issues:

  • Typos on syncBinding and asyncBinding
  • Couldn't find an npm test script for CI as stated, but found the shell script and updated the docs incorrect 😑
  • Added an executable bit to the shell script so it can be invoked directly

Let me know if you're ok with this!

@justadudewhohacks
Copy link
Owner

Thanks!

Couldn't find an npm test script for CI as stated, but found the shell script and updated the docs

What happened when you invoked the test script as specified via npm? npm run test 3.4.6-contrib 11 should simply call the script via bash command.

@foxxyz
Copy link
Contributor Author

foxxyz commented Aug 24, 2019

You're right. I'm not sure how I overlooked this... 🤔

I've removed that part of the doc update, I think the rest is correct.

As an aside, I wish the CI/docker builds could somehow also be called via npm test from the /test directory, instead of having to go to /ci/test first and run npm test there, but I don't actually have a good solution for that. Just a thought!

@justadudewhohacks
Copy link
Owner

As an aside, I wish the CI/docker builds could somehow also be called via npm test from the /test directory, instead of having to go to /ci/test first and run npm test there, but I don't actually have a good solution for that.

Yeah, I think you would add a script to the base package.json calling the shell script but also setting the current working directory to the ci folder.

@justadudewhohacks justadudewhohacks merged commit b497f7a into justadudewhohacks:master Aug 24, 2019
@foxxyz
Copy link
Contributor Author

foxxyz commented Aug 24, 2019

Thank you so much! 🎉

@foxxyz foxxyz deleted the docfix branch August 24, 2019 19:03
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.

2 participants