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

[WIP] added pycodestyle checking and ci structure #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adamsxs
Copy link

@adamsxs adamsxs commented Sep 23, 2019

Fixes: #46

Added pycodestyle and moved CI components into a .ci folder as suggested by @jameslamb in the provided example.

@adamsxs
Copy link
Author

adamsxs commented Sep 23, 2019

[Feedback Request] pycodestyle triggers on substantial amount of code. I've already added E501 to the tox.ini file. Would you all think it'd be better to include those changes in a separate PR or as part of this one?
Also, should package installs get moved from travis.yml to .ci/setup.sh?
(EDIT: Based on failing build below, code style changes may be needed (It's now using the new .yml file. Should be able to test this by commenting out the pycodestyle bit and see if the builds still fail. If they do, code needs to be linted.)

@bburns632
Copy link
Contributor

@adamsxs , since this PR w/o those changes would make master fail upon merge, I think including those changes in this PR would be best.

Also, @adamsxs, how will style errors present themselves: as warnings in the PR or as a failed unit test?

@adamsxs
Copy link
Author

adamsxs commented Sep 23, 2019

@bburns632 Currently? They are presenting themselves as a failed build (see above). It's currently executed before pytest, so don't think it counts as a unit test per se. Definitely more serious than a warning though.

In the future? Depends on how aggressively you want to enforce conventions. I think the easiest thing for maintainers, and the best for consistency across the code base, would be to keep them as errors. Based on the length of the logs from the previous builds, it's probably worth figuring out which ones are taking a stand on (errors) and which PEP8 conventions are less valuable.

@bburns632
Copy link
Contributor

bburns632 commented Sep 23, 2019 via email

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.

Add checks on codestyle in CI
2 participants