-
-
Notifications
You must be signed in to change notification settings - Fork 314
Add CONTRIBUTION guide #205
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
Conversation
|
|
||
| Please follow [PEP8](https://www.python.org/dev/peps/pep-0008/) guide. | ||
| See [Running Test](./README.md#running-tests) and [Linting](./README.md#linting) for further instructions. | ||
|
|
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 we add a step here about running the formatter?
pipenv run fmt or make fmt or black . -l 120
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.
Thanks for feedback. Added make ftm to styleguide section.
|
Some points to discuss? :-) |
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 contribution sections looks good 😄.
A couple of issues with the coverage changes. IMO they should also be part of a separate PR.
| install: | ||
| - "pip install pipenv" | ||
| - "pipenv install --dev --skip-lock" | ||
| - "pipenv install coveralls" |
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.
IMO this coveralls stuff should be a totally separate PR but...
You shouldn't use pipenv install this dependency here.
- Either add it to the
Pipfileso that it gets installed along with thedevdependencies - install it directly with pip in the first install step.
pip install pipenv coveralls
| pytest-cov="*" | ||
| pylint = "*" | ||
| coveralls = "*" |
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.
Updates to the Pipfile should always be accompanied by a new Pipfile.lock
Run pipenv lock to regenerate a new lockfile.
However, if you are on a windows machine it may not generate the correct dependencies for a deployment. If so let me know and I would be happy to generate it for you,
|
@gribok Can I remove the coverage stuff from this branch and merge it? |
Based on #169, provides first version of contrib guide.