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

style: run github.com/psf/black #554

Merged
merged 3 commits into from
Oct 13, 2019

Conversation

eine
Copy link
Collaborator

@eine eine commented Sep 24, 2019

This is a proof of concept about adding a default python formatter.

pip3 install black
black ./

@eine eine force-pushed the feat-py-formatter branch 5 times, most recently from 88d3a38 to 6c6686f Compare September 25, 2019 17:33
@kraigher
Copy link
Collaborator

It is a good idea, I assume they solved the problem that horizontal indentation in python is sematically relevant since it defines the scope of if and for loops.

@eine
Copy link
Collaborator Author

eine commented Sep 25, 2019

They did. black produces PEP8 compatible code, although some things are incorrectly detected as non-compilant by either pycodestyle or pylint. These are known bugs, and I have added a fix for them: 6cc7888

The main discussion we need to have regarding this issue is: do we want to use a heavily opinionated formatter (black) so that we cannot have any discussion about style at all? Or do we want to use yapf, which requires us to decide the main flavour and multiple tweaks/settings? Please, see the article linked above.

At first sight, I prefer yapf (Facebook) to black. However, precisely because I feel I prefer one of them, I decided to open this PR using black.

@kraigher
Copy link
Collaborator

My opinion about style and formatting is that is one of the most low value things to argue about :-)
The important thing is that the style is consistent in the code base and preferably in the entire language ecosystem. That there even is a need to argue about style just points to a flaw in the ecosystem of the language.

Most modern languages have autoformatters precisely because there should be one and only one way since it does not matter (within reason) how it looks just that it is the same for all developers. Python was one of the early languages to think in this way with PEP8 but they did not create an autoformatter rather a checker. Autoformatter is better then a checker since it automatically fixes the issues. I really like rustfmt when coding Rust which works the same.

@eine
Copy link
Collaborator Author

eine commented Sep 25, 2019

We completely agree :D. It is almost annoying to work with Python, VHDL or C, once you get used to prettier, gofmt, rustfmt, etc.

Nevertheless, let's wait until we publish the next release before merging this. On the one hand, we won't disturb the PRs that are 'milestoned'. On the other hand, it will give me some time to add it to the CI pipelines.

@kraigher
Copy link
Collaborator

This is ideally something you do when there is not a lot of Python changes outside of master in various PR:s.

@LarsAsplund
Copy link
Collaborator

LarsAsplund commented Sep 26, 2019

I'm all for introducing this. If we want to keep style discussions to a minimum we should avoid tools allowing configuration so Black seems to be a good choice. It's also developed by one of the Python core developers and has the best momentum. If there will be a future Python standard this is probably the best bet.

@kraigher
Copy link
Collaborator

Now is a good time to do this. It should be added as a test to CI like pycodestyle and pylint though.

@eine eine force-pushed the feat-py-formatter branch 6 times, most recently from bad49e5 to 58f5fd6 Compare October 13, 2019 03:14
@eine
Copy link
Collaborator Author

eine commented Oct 13, 2019

Now is a good time to do this. It should be added as a test to CI like pycodestyle and pylint though.

I updated the PR and I added py*-fmt commands to tox.ini. Developers/contributors can use e.g. tox -e py37-fmt to have the code formatted. Moreover, a I added a Travis job that executes tox -e py37-fmt -- --check. This will fail instead of modifying sources. black requires >=3.6, so I added a single fmt job with py37.

Note that subdir vunit/vhdl/JSON-for-VHDL is explicitly excluded from black. This is because it is a submodule, and we cannot format python sources there.

@eine eine marked this pull request as ready for review October 13, 2019 03:33
This pull request was closed.
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.

3 participants