-
Notifications
You must be signed in to change notification settings - Fork 512
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
Add Pull Request template #647
Conversation
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
||
**Checklist**: | ||
- [ ] My PR is ready for code review | ||
- [ ] I have added some tests, if applicable |
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 would add, '"and run the whole test suite"
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 would also add "even linting tests, this way we can get rid of the next point"
.github/PULL_REQUEST_TEMPLATE.md
Outdated
**Checklist**: | ||
- [ ] My PR is ready for code review | ||
- [ ] I have added some tests, if applicable | ||
- [ ] My code respects the [PEP8 style guide](https://www.python.org/dev/peps/pep-0008/) |
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 don't know if that is necessary. I think that for now we have no guaranty that our current code base is PEP8 compliant. The only thing we know is that the codebase is compliant with the Google-like linting rules that we have in the pylintrc
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.
Plus, I find it very hard to know if your code is PEP8 compliant from juste the guidelines provided in the URL. The linting test has the advantage to tell you precisely what's wrong
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.
Indeed
No description provided.