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

Add python auto-formatting tooling #3609

Closed
jonboiser opened this issue May 2, 2018 · 4 comments · Fixed by #5333
Closed

Add python auto-formatting tooling #3609

jonboiser opened this issue May 2, 2018 · 4 comments · Fixed by #5333
Assignees
Labels
TAG: dev experience Build performance, linting, debugging...

Comments

@jonboiser
Copy link
Contributor

jonboiser commented May 2, 2018

In thread below, there is a desire to use something like

  • autopep
  • black
  • yapf

To auto-format the python codebase.

Old issue:

Don't know if it's a coincidence, but I'm noticing the flake8 config in `setup.cfg` ``` [flake8] ignore = E226,E302,E41 max-line-length = 160 max-complexity = 10 exclude = kolibri/*/migrations/*,kolibri/dist/* ```

is really similar to the docs here:

http://pycodestyle.pycqa.org/en/latest/intro.html#error-codes

And E41 does not seem to be a pycodestyle code either.

E226 (requires whitespace around math operators) and E302 (two newlines between top-level symbols in a module) seem like things that are done the majority of the time and all of the time for me personally. So maybe we should just remove the ignore settings

@jonboiser jonboiser added the TAG: dev experience Build performance, linting, debugging... label May 2, 2018
@bonidjukic
Copy link
Contributor

@jonboiser I was about to open a new issue regarding the Python code style in Kolibri in more general terms but I guess this might be a good opportunity to start the discussion with others.

I think it would be great if we could review the current flake8 config and maybe configure some tool(s) to automatically reformat the code before or after commits, because as far as I understand currently there is no workflow which would enforce the code standards on the backend.

My personal thoughts:

  • max-line-length = 160 - I read somewhere in the Kolibri documentation that this was defined with the intent of avoiding unnecessary code wrapping, but I believe that this may actually lower the code readability.
    Not moving strictly to the PEP8 advised 80 limit, I believe that 120 limit should be more than enough to satisfy most line length arguments.
  • single vs. double quotes - not sure if this could be reformatted with tools automatically (as it could introduce bugs), but I believe it would be a good idea to be more consistent when dealing with quotes - currently there are situations where different quotes are being used in two subsequent lines of code.
  • I agree with @jonboiser that we should remove the ignore directive from the setup.cfg (Note: E41 means that all E41* codes will be ignored, but since currently there are no E41* codes, that's a noop)

@rtibbles previously suggested that we could use autopep8 to help with automatic reformatting - maybe adding it to the existing pre-commit hook?

@rtibbles
Copy link
Member

rtibbles commented May 3, 2018

I agree with this, especially if we use autopep8 as a precommit hook, I would suggest that we do this as the first step in the 0.11 backend refactor work.

@rtibbles
Copy link
Member

rtibbles commented May 3, 2018

@indirectlylit suggests YAPF https://github.com/google/yapf

@rtibbles rtibbles added this to the Summer 2018 Refactor milestone May 8, 2018
@indirectlylit
Copy link
Contributor

updating my recommendation to https://github.com/ambv/black

@jonboiser jonboiser changed the title Don't ignore any violations in the flake8 config Add python auto-formatting tooling Jul 10, 2018
@indirectlylit indirectlylit removed this from the Pre-0.11.0 upgrades milestone Jul 23, 2018
@indirectlylit indirectlylit self-assigned this Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TAG: dev experience Build performance, linting, debugging...
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants