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

Feature/update contributor and readme #185

Merged
merged 3 commits into from
Dec 21, 2016

Conversation

DanielaValero
Copy link

@DanielaValero DanielaValero commented Dec 9, 2016

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Add note about rewrite in REAME and setup for contributors with yarn plus explicit note about good commit messages.
Relates to: #184

Does this PR introduce a breaking change?

  • Yes
  • No

Other information: N/A

@jsf-clabot
Copy link

jsf-clabot commented Dec 9, 2016

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Daniela Valero seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@DanielaValero DanielaValero force-pushed the feature/update-contributor-and-readme branch from fcccbc1 to b339af1 Compare December 9, 2016 17:54
@DanielaValero
Copy link
Author

Copy link
Contributor

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

If you want to add yarn instructions, I would add them addition to those for npm. Less people use yarn than you might think and more importantly, the project itself doesn't use yarn for script execution.

As a lib, imo the tasks should execute as npm ( which it does ). We can provide standard docs for npm & additional documentation for those that want to use yarn.

They way this looks in it's current state, development on karma-webpack requires the use of yarn, which is not the case.

Outside of that it's g2g.
//cc @MikaAK

@DanielaValero DanielaValero force-pushed the feature/update-contributor-and-readme branch from b339af1 to c4035d4 Compare December 12, 2016 19:49
Daniela Valero added 3 commits December 12, 2016 14:49
After having added yarn to our build system, the
setup steps for contributors need also to be
updated to be done with yarn instead of npm.
We do want to generate a meaningful history, so
when we look back in the past the commit messages
help us to understand what was done by the
commit. This requires lots of training, and
explicit mentions in our contributors guidelines.
In order to enable users to be aware about our
rewrite, and still be able to see the code of
karma-webpack, adding a note in our README.
@DanielaValero
Copy link
Author

Hi @d3viant0ne

Very good point, I have updated the PR with the changes you've requested.

@DanielaValero
Copy link
Author

Hi @d3viant0ne @MikaAK do we have a special workflow to merge a PR ? for example X amount of approvals before merge?

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.

4 participants