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

Reformat the code using black, enforce black code formatting #5156

Merged
merged 23 commits into from
Mar 6, 2021

Conversation

Kami
Copy link
Member

@Kami Kami commented Feb 17, 2021

This pull request reformats all the code using black tool and also adds a corresponding Make target and CI check which enforces code contains black compliant style / formatting.

To prevent large non functional formatting only change being attributed to myself and confusing git blame, etc. I made that change under a different author (8496bb2).

I had quite some issues trying to black to pass on all the files - I needed to manually fix formatting in files first before black could proceed (on some files it would fail with an error that second pass produced different result than the first one).

Open PRs

There is no way around it - such change will always cause some issues with merge conflicts and open PRs. But the sooner we do it the better,

The easiest way to get that resolved is for people who have opened PRs to run black on their code + changes and then merge latest master with formatting changes into that branch - this should result in least amount of merge conflicts.

So something like this (once this PR is merged):

# Inside StackStorm development virtualenv
curl -O /tmp/pyproject.toml https://raw.githubusercontent.com/StackStorm/st2/master/pyproject.toml 
pip install "black==20.8b1"
black --config /tmp/pyproject.toml *

TODO

  • Update documentation (coding style)
  • pre-commit git hook which makes it easier to run flake8, pylint and black only on changed files
  • Document how to use pre commit config
  • Update lint-configs repo instead of in-line (once black is applied on all the repos which utilize those configs - we can't do it before that because some flake8 rule exclusions rely on black being present)
  • Guide for people who have outstanding PRs - we should likely prepare a template we can post as a comment on open PRs, if needed
  • Reformat all the packs code and add checks to packs CI (TODO for the future)

@Kami Kami added this to the 3.5.0 milestone Feb 17, 2021
@pull-request-size pull-request-size bot added the size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. label Feb 17, 2021
@CLAassistant
Copy link

CLAassistant commented Feb 17, 2021

CLA assistant check
All committers have signed the CLA.

@cognifloyd
Copy link
Member

Oh please! I have been reformatting files with black, and then manually adjusting the diff so that I only submit PRs for lines I've touched.

I really hope we can use it in the exchange as well, because lint configs are difficult to use while developing a pack. It will be so much simpler when we standardize on black, dropping most of the lint config files.

Black does not handle some things, like import alphabetizing, but it is very consistent for everything else.

@Kami Kami added this to In progress in StackStorm v3.5.0 via automation Feb 17, 2021
@cognifloyd
Copy link
Member

LOL! the bot hasn't signed the CLA. ha ha ha.

❌ stackstorm-neptr

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

LGTM. I probably wait until post v3.4 release to merge this.

@cognifloyd
Copy link
Member

I recommend we add a .git-blame-ignore-revs file as described here:
https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256

Then, hopefully when github adds support we will be set to benefit from it.

@Kami
Copy link
Member Author

Kami commented Feb 17, 2021

@cognifloyd Yeah, that works for me.

I also pushed a pre-commit config (705d585) which runs all the lint checks (flake8, pylint, black) on modified / added files when committing - this should come very handy during development since it's much faster than running those checks on the whole code base (there should be no need to do that when developing locally since the base copy aka non modified files should be clean).

I will add some info to the docs on how to use it.

@Kami Kami requested review from a team as code owners February 17, 2021 23:26
@Kami
Copy link
Member Author

Kami commented Feb 17, 2021

While at it, I also added two additional pre-commit checks which verify filess contain no trailing whitespace and that all the YAML files are valid + hooked it up to the make targets + CI.

@Kami
Copy link
Member Author

Kami commented Feb 17, 2021

For a response template for existing PRs, we can use something like this:

We have recently merged a PR which automatically reformats the code using black tool - #5156.

We have done this to ensure consistent code formatting and style for everyone.

Since your PR was created before #5156 was merged, this means you will likely encouter merge conflicts when merging latest master into your branch.

Sadly there is no way around that, but to make it easier, we recommend running the following commands / stpes:

# Inside StackStorm development virtualenv
curl -o /tmp/pyproject.toml https://raw.githubusercontent.com/StackStorm/st2/master/pyproject.toml 
pip install "black==20.8b1"
black --config /tmp/pyproject.toml *
# Commit the files you modified, but not the ones you didn't (there will be a lot of modified files shown since black will run on all the files in the repo)
git add file 1 file 2 ...
git commit -m ....
# merge latest master into your branch
git merge master

This will ensure the files you changed will be already formatted using black before merging latest master in your branch. This should greatly reduce the likelihood of a merge conflict.

@cognifloyd
Copy link
Member

For PRs that allow commits from maintainers, we can also push the reformat.

@Kami
Copy link
Member Author

Kami commented Feb 18, 2021

Please let me know when v3.4.0 is out so I can merge this to avoid more conflicts on my side.

@Kami Kami merged commit 7c8ecd7 into master Mar 6, 2021
StackStorm v3.5.0 automation moved this from In progress to Done Mar 6, 2021
@Kami Kami deleted the reformat_code_using_black branch March 6, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants