-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
feat: implement issue and pr templates #641
Conversation
I think we should also mention the JavaScript linter in the PR template. |
Agree with @frangio. I'd actually replace the style guides item in favour of the linter. |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] π I've made sure that my contracts are well-documented. | ||
- [ ] π¨ I've run the JavaScript linter (`npm run lint`) and fixed all issues. | ||
|
||
**Does this close any open issues?** If so, list them here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this is that the contributor will not write a phrase like "fixes #PR" which is what GitHub uses to close said issue automatically when the PR is merged. How can we nudge the contributor towards that?
Does this close any open issues? I so, list them here.
Fixes #...
Also, should this line be an HTML comment?
.github/PULL_REQUEST_TEMPLATE.md
Outdated
@@ -0,0 +1,16 @@ | |||
π Thank you for submitting a PR! Before submitting, please review the following checklist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this line also be an HTML comment?
@@ -80,6 +80,8 @@ git checkout -b fix/some-bug | |||
git checkout -b remove/some-file | |||
``` | |||
|
|||
If your branch is planned to fix an open issue, postfix your branch name with the issue number like `fix/some-bug-#123`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this was done in this PR but Waffle didn't associate it to the corresponding issue correctly. It may be related to the lack of "fixes #639" thing mentioned above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frangio did we switch to solium or what, yet? Do we have a process for linting the solidity files? I see a soliumrc, but not sure how we invoke solium officially. It should be an npm script. ( and I've updated the PR description above to the most recent push) |
74014e0
to
29edd4e
Compare
Solium was set up once a long time ago but at some point we stopped using it. There is #556, we should merge that and add an npm script, and probably run it in Travis. |
β¦#639 feat: implement issue and pr templates
npm run lint:fix
) and fixed all issues.Fixes #639
π Description
Adds issue and pr templates as well as updates the contributing doc slightly.