Add checks on codestyle in CI

[jameslamb]

Add pycodestyle to the CI setup for this project to prevent PRs from introducing style issues and to document the preferred style for this project.

See this example PR for an example of how to add this type of check to your build. Basically you need to run pycodestyle command on the repo and can optionally configure what it checks with a tox.ini file.

Docs here: http://pycodestyle.pycqa.org/en/latest/intro.html

[SharathGa]

@jameslamb @adamsxs @bburns632
I would like to work on this issue, if it is still not resolved.

Suggestion: Use Pre-commit hooks to format and check the python code and keep the CI file the same.
Solution:

• Use tools like Black for style formatting and Flake8 as a checker for the formatting instead of using the unit tests to check the formatting (as @bburns632 suggested in the PR [WIP] added pycodestyle checking and ci structure #50 by @adamsxs). Using these tools will also help us when we scale up and contributors/maintainers can focus on the code logic and implementation, rather than the formatting.

• Also this will allow us to keep the .travis.yml file the same hopefully.
  Please take a look into This article about using pre-commit hooks using black and flake8

If everyone is okay with the proposal, I would be glad to start working on this issue.

#Hacktober2019

[bburns632]

Hi @SharathGa ,

See comments in #50 and @adamsxs, can you confirm that PR is no longer in progress?

In short:

1. A PR to add an automated code style policy must also change the current code to align with that policy. Otherwise, every future PR to master would fail.
2. Because of the above, I recommend we start with code style policies that impact the current code minimally.

For ease of DIFF review (read: reviewer sanity), please keep PRs small.

[SharathGa]

Hi @bburns632

Proposed solution:

1. I can manually change the formatting of the individual files whilst keeping each PR smalls for the DIFF review using Black. It is in accordance to PEP8 rules, but its most important focus is code readability.

2. Once we do the formatting of the existing Codebase, we can set up the pre-commit hooks such that the future PRs to master wont fail.

What do you think?

[bburns632]

Hi @SharathGa ,

I like how @gsganden has managed github.com/uptake/autofocus, so I would like his opinion on default python PR code style checks in addition to @jameslamb.

[jameslamb]

I personally do not like pre-commit hooks because they force people to have additional software (e.g. black in this example) installed in their development environments.

I the idea of not doing this all at once in one PR. I would propose the following:

• PR 1: set up the checks to run during CI and only have them look for a single style issue (say, lines that are more than 100 characters long)
• PR 2: once PR 1 is merged, propose addition of a few more style things + fix them
• etc., etc. until there are none left that you want to check for

I don't want / have any authority over the ultimate decision made for this repo, that's just my personal opinion on handling this.

Thanks for being patient with us and contributing @SharathGa !

[gsganden]

FWIW, Autofocus checks for conformity to both flake8 and black. Rather than using a pre-commit hook, we ask contributors to ensure conformity to both tools as part of the PR process. I am going to change the flow so that the contribute to a dev branch. That way if they aren't comfortable with flake8 and black then I can go ahead and merge their contribution into dev and then fix up the style myself before merging into master.

FWIW, I would make the changes in one fell swoop. black does 90% of the work, so it shouldn't be too hard.

[SharathGa]

@jameslamb my pleasure! it feels great to get quick reponse and valuable feedback from the community @bburns632 @Chronocook.
Looking forward to collaborate more with you guys here.

@gsganden makes sense , this is also sort of how I am working on multiple issues of numpynet. I have separate branches for each issue and I merge them with my master when the branch into pulled in the main master .
Also any suggestions on optimizing my branching workflow in order to work on several issues at once?

@jameslamb @bburns632
I can see the idea of not using too many dependencies, And using the dev branch also makes a lot of sense to me. At this point, I am open to any logical direction the codeowners want the codestyle of this project to be. Happy to help!

[jameslamb]

It looks like this project is abandoned. Closing this.