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

Code style: Black, autopep8, or yapf? #288

Closed
banesullivan opened this issue Jul 2, 2019 · 11 comments
Closed

Code style: Black, autopep8, or yapf? #288

banesullivan opened this issue Jul 2, 2019 · 11 comments
Labels
discussion Thought provoking threads where decisions have to be made proposed-change Something with regards to the API or internal structure is changing.

Comments

@banesullivan
Copy link
Member

banesullivan commented Jul 2, 2019

I'm playing around with Black and it's super easy to integrate into projects with a pre-commit hook.

I'm curious how the active contributors would feel about embracing an automatic code-styler like Black (or another)? This would give us all more consistent formatting of code across the API. It's also designed to produce easier-to-navigate diffs and remove the need for formatting by hand.

It looks like each contributor would have to set up the pre-commit hook I think?

Ping @akaszynski

@banesullivan banesullivan added discussion Thought provoking threads where decisions have to be made proposed-change Something with regards to the API or internal structure is changing. labels Jul 2, 2019
@banesullivan banesullivan changed the title Code style: Black? Code style: Black, autopep8, or yapf? Jul 2, 2019
@banesullivan
Copy link
Member Author

Interesting resource and alternative to Black: https://github.com/kenneth-reitz/white

@akaszynski
Copy link
Member

I agree with adding a coding style, and black sounds good to me. I don't think we must adhere to 79 characters (the default 88 in black works), though we should keep most lines within 79 and use 88 as the exception to the rule.

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Oct 23, 2019

After more thoughts, I can only push towards integrating the checkers in the CIs (pydocstyle, flake8, ...) instead of using automatic code formatters. It's less constraint for the contributor, it doesn't achieve any magical, out of control, code formatting, the commits stay relevant (feature vs style) and it can even raise awareness on standard coding practice.

I volunteer to start a PR configuring some Travis to do that and of course it will complain for a time but once this is done, the code can only be more readable and pleasant for everybody. The sooner it is done, the better I think. Work has already started in this direction in #408

@leouieda
Copy link
Contributor

Not really a contributor here but from my own experience using Black really makes the contribution process a lot easier. I don't have to try to figure what style the project uses. I run black and it's done. As a maintainer, I don't have to comment on silly code style (break lines here, indentation after parenthesis, etc). I can tell people to run black and all is fine. So it's one less thing to think about and I can focus the review on the important stuff: tests, variable names, proper use of types, documentation, design.

The one caveat are docstrings. I made the mistake of making them 88 chars as well. In Jupyter/IPython, that means that docstrngs are basically unreadable. Black doesn't touch docstrings so it's not a huge problem.

@tkoyama010
Copy link
Member

tkoyama010 commented Dec 23, 2019

@GuillaumeFavelier Thank you for your mention. I'm also not a developer, but I'm +1 for black. And ⬆️ is my patch and opinion. And also isort is good to use. Thank you for reading.

@banesullivan
Copy link
Member Author

isort is really awesome - I manually use it on PyVista every once in a while as imports do not change that often

@tkoyama010
Copy link
Member

FYI it became major to add type annotation to python code. It maybe good to add type annotation and check by mypy (from Oh my(py) and mypy).

@GuillaumeFavelier
Copy link
Contributor

Any news on this one? :)

@tkoyama010
Copy link
Member

Now we use pylint, pycodestyle, flake8, black, mypy, isort in pyvistaqt.
pyvista/pyvistaqt#38
pyvista/pyvistaqt#40
pyvista/pyvistaqt#43
pyvista/pyvistaqt#45
pyvista/pyvistaqt#46
pyvista/pyvistaqt#47

@akaszynski
Copy link
Member

We probably can close this issue then. Thanks for pointing this out @tkoyama010

@banesullivan
Copy link
Member Author

We probably can close this issue then

agreed. I think we've converged on which things to use if we go that route so I will close this issue.

It may be a long time before we actually follow through with an autoformatter here though (just such a large codebase to try to do it all at once)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Thought provoking threads where decisions have to be made proposed-change Something with regards to the API or internal structure is changing.
Projects
None yet
Development

No branches or pull requests

5 participants