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

docs(contributing): improve formatting #768

Merged
merged 2 commits into from
Mar 10, 2019

Conversation

misterdev
Copy link
Contributor

What kind of change does this PR introduce?
Improves formatting of the Setup instructions in CONTRIBUTING.md

Before:
screenshot 2019-03-01 at 20 20 51

After:
screenshot 2019-03-01 at 20 18 57

@ematipico
Copy link
Contributor

Could you please remove the third bullet point? It's old documentation and code

@misterdev
Copy link
Contributor Author

Hi @ematipico, I've removed the third bullet point, now the setup section looks like this:

screenshot 2019-03-02 at 11 49 37

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for submitting a PR!

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Could you rebase against master and fix any formatting errors? Thanks!

@webpack-bot
Copy link

@misterdev Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evenstensberg Please review the new changes.

@ghost
Copy link

ghost commented Mar 9, 2019

There were the following issues with this Pull Request

  • Commit: 41c93ba
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented Mar 9, 2019

There were the following issues with this Pull Request

  • Commit: 41c93ba
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@misterdev misterdev force-pushed the docs/contributing-formatting branch 4 times, most recently from 0a41fb9 to 94387e7 Compare March 10, 2019 01:02
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved

* To test a single CLI (other type of) test case:
- `yarn jest path/to/my-test.js`
* You can also install jest globally and run tests without npx:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't suggest to install a package globally. Npx is enough as it comes for free and there's no installation process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been introduced by #777

Copy link
Contributor

Choose a reason for hiding this comment

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

So why it is marked as an addition in this PR? Did you rebase your branch?

Copy link
Member

Choose a reason for hiding this comment

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

It's for people that want to reuse a scaffold @ematipico instead of re-installing every time. I don't think npx will completely solve that issue

Copy link
Member

Choose a reason for hiding this comment

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

IMO Reusing a scaffold is related to @webpack-cli/init being able work with it's global installation @evenstensberg.

I think for this issue we should just write if the user has global installation of jest then we should run jest without npm for test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ematipico I moved this code to separate setup and testing, it is marked as deleted some lines above

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants