Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Mar 19, 2019

We've talked about this for a while now and it's time to finally make some concrete steps to consistent code style and automatic code formatting.

Background / Context

We, the developers, have various opinions on code styling which are by nature, subjective. Some of the more "objectively" defined style rules (defined in thing such as pep8, etc.) are already enforced in our code base using tools like pep8, flake8 and pylint, but those tools only go so far.

Besides that it's a bit of "wild west" - we all have our own preferences and those tend to end up in the code base one way or another.

This means inconsistent styling in the code which makes the code harder to follow, read and understand, etc. It also means unnecessary "arguing" about style choices.

To solve for that and have a single consistent style in the code (think gofmt), we should introduce a tool which automatically and deterministically formats the code so it follows a single consistent style,

After doing some research, I think the best tool for our use case probably is black - https://github.com/ambv/black.

There are other tools out there, but the good thing about black is that it's opinionated. If we select a tool which has 100 different options and flag to tweak styling to our linking (think yapf - https://github.com/google/yapf), then we will probably never all agree on how those options should be configured.

And personally, I'm also not a fan of some of the styling choices black makes, but at least it's consistent.

Once we agree on the approach and library to use, we should make the following changes:

  1. Reformat all of the existing code - initial diff will be large, but there is no way around that
  2. Document this in our developer documentation, contribution guidelines, etc.
  3. Automatically enforce reformatting as part of:
    • Pre-commit hook
    • Travis CI build process - fail the build if code hasn't been formatted using black style
    • Document how people can integrate and do this automatically inside their editor / IDE
  4. Eventually enforce the same styling rules on StackStorm-Exchange repos

Kami added 3 commits March 19, 2019 16:45
e0a12db78 Add config for black tool.

git-subtree-dir: lint-configs
git-subtree-split: e0a12db7845471b41db5cc4231880fef44c641f2
-e{toxinidir}/st2reactor

# Python 3 tasks
[testenv:py36-black]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I needed to add a special tox target because black only works under Python 3.

Kami added 2 commits March 19, 2019 17:20
41e64995b Add config for black tool.
REVERT: e0a12db78 Add config for black tool.

git-subtree-dir: lint-configs
git-subtree-split: 41e64995bd25c8333806aacfaebaf998017232c2
@Kami
Copy link
Member Author

Kami commented Mar 19, 2019

Here is an example of the code base being reformatted using black - https://github.com/StackStorm/st2/compare/black_auto_code_format...black_auto_code_format_reformatted?expand=1.

@Kami
Copy link
Member Author

Kami commented Mar 19, 2019

And here is a short list of quick examples of things I don't like / things it's lacking:

  1. I find this much less readable compared to the original version - https://github.com/StackStorm/st2/compare/black_auto_code_format...black_auto_code_format_reformatted?expand=1#diff-97d2525fd77b27fc5275d98eee8d36c1L57
  2. It doesn't do anything about consistent import ordering and styles

This means other options like yapf might still be a valid choice.

@m4dcoder
Copy link
Contributor

m4dcoder commented Mar 19, 2019

Looking at the st2 reformatted example, it fixes some of the coding I would do. One of my biggest complaint about st2 styling are the imports (inconsistent import of modules and classes and functions, direct import of functions, import not in alphabetical orders, inconsistent naming of import alias). I can give my reasoning for each of them. So if black doesn't automatically take care of that, then we need to agree on a separate tool or agree on what those rules are.

@Kami
Copy link
Member Author

Kami commented Mar 20, 2019

I agree about imports, that's also one of the limitations I mentioned on Slack.

Those things are not mutually exclusive though and there are other tools we should look at. For example - https://github.com/PyCQA/flake8-import-order

And perhaps we should also start with import re-order since diff should be a bit smaller :)

@stale
Copy link

stale bot commented Jun 18, 2019

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Jun 18, 2019
@stale stale bot removed the stale label Jan 29, 2020
@stale
Copy link

stale bot commented Jan 29, 2020

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Jan 29, 2020
@stale stale bot removed the stale label Apr 1, 2020
@stale
Copy link

stale bot commented Jul 1, 2020

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Jul 1, 2020
@cognifloyd
Copy link
Member

Obsoleted by #5156. Please close.

@stale stale bot removed the stale label Mar 7, 2021
@arm4b
Copy link
Member

arm4b commented Mar 8, 2021

Thanks, @cognifloyd for tracking this 👍

@arm4b arm4b closed this Mar 8, 2021
@arm4b arm4b deleted the black_auto_code_format branch March 8, 2021 22:32
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.

5 participants