Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Fixes #57 runs black & isort for code formatting check #61

Merged
merged 3 commits into from Jun 4, 2020

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Jan 17, 2020

Ready for review

Fixes #144.

Adds new lint targets for black and isort tool.

make lint will now first check any isort change and then black
and then flake8. All 3 commands use line length of 100.

The last commit also adds a .git-blame-ignore-revs file,
so that we don't see the changes from the formatting commit in the
git blame output.

How to test manually?

  • go back to the first commit in this PR git checkout f9bcbd3e8b49b737b2b85bf608f48349ddc68a4b
  • make black
  • make isort
  • make lint

All of the above make commands should fail as formatting change is required.

  • move to next commit git checkout 787844dc339e090aa0e9ac2241895365522c4119
  • make black
  • make isort
  • make lint

All of the make commands should now pass. Please note down a few lines in the different files changed in this commit for the next test. Use git blame to get the details.

  • move to the latest commit git checkout 32e62719148d54d1c264e8b781056803fa46b24a
  • git config blame.ignoreRevsFile .git-blame-ignore-revs run this command.
  • git blame should not my name in the lines changed in the last commit.

extra notes

Here is one fail message from black.

$ make lint
would reformat /home/kdas/code/securedrop-proxy/securedrop_proxy/proxy.py
Oh no! 💥 💔 💥
1 file would be reformatted, 8 files would be left unchanged.
make: *** [Makefile:32: black] Error 1

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

Looks good to me, but Black requires Python >= 3.6, so we should probably update setup.py to match. With a 3.5 virtualenv, pip won't be able to find Black.

Also, for some reason the Unicode flair is displayed as tofu in my Qubes development VM, though I tried several fonts and a couple of terminal emulators, some combinations of which definitely worked on regular Debian. Not a blocker. 🤷‍♂️

@eloquence eloquence added this to Ready for Review in SecureDrop Team Board Jan 17, 2020
@kushaldas
Copy link
Contributor Author

Looks good to me, but Black requires Python >= 3.6, so we should probably update setup.py to match. With a 3.5 virtualenv, pip won't be able to find Black.

I am marking it as Python >= 3.7 as we are running and testing on 3.7 in Debian Buster.

@kushaldas kushaldas force-pushed the run_black_run_in_ci_and_on_dev branch 2 times, most recently from 1fc38c4 to 09a81cd Compare January 20, 2020 05:14
@rmol
Copy link
Contributor

rmol commented Jan 21, 2020

Python version requirement looks good, but I realized something else: our Flake8 configuration uses a line length of 100. For now we should probably add a pyproject.toml adjusting Black's line length to match our current convention.

@eloquence eloquence moved this from Ready for Review to In Development in SecureDrop Team Board Jan 21, 2020
@kushaldas kushaldas force-pushed the run_black_run_in_ci_and_on_dev branch from 09a81cd to 29d2047 Compare January 23, 2020 09:42
@kushaldas
Copy link
Contributor Author

kushaldas commented Jan 23, 2020

@rmol I added pyproject.toml and also a new commit after running black on the source.

Note: except securedrop none of the other projects have any flake8 configuration from our side.

@kushaldas
Copy link
Contributor Author

Okay, CI is failing to build the package because pip/setuptools is using PEP 517 to build via pyproject.toml, means we can not have it here. Instead I will add .flake8 and mark the maximum line length to 88 as in default black.


10:22:13 | <pradyunsg> | dh_virtualenv...
10:22:19 | <pradyunsg> | pip._internal.exceptions.DistributionNotFound: No matching distribution found for setuptools>=40.8.0
10:22:19 | <pradyunsg> | Installing build dependencies ... error
10:22:38 | <pradyunsg> | It's trying a PEP 517 build for some package that has a pyproject.toml.
10:23:00 | <pradyunsg> | And I don't think dh_virtualenv was ever updated for this.

@rmol @redshiftzero

@kushaldas kushaldas force-pushed the run_black_run_in_ci_and_on_dev branch 3 times, most recently from dacac59 to 98193e3 Compare January 23, 2020 10:42
@eloquence eloquence moved this from In Development to Ready for Review in SecureDrop Team Board Jan 23, 2020
@eloquence
Copy link
Member

We agreed to hold off until this change for now until a cross-repo code style standardization conversation, which is scheduled for after the SecureDrop Workstation pilot launch.

@eloquence eloquence removed this from Ready for Review in SecureDrop Team Board Jan 30, 2020
@eloquence eloquence added this to Nominated for next sprint in SecureDrop Team Board Jun 3, 2020
@eloquence eloquence removed the blocked label Jun 3, 2020
@eloquence eloquence moved this from Nominated for next sprint to Sprint #52 - 6/3-6/17 in SecureDrop Team Board Jun 3, 2020
@eloquence
Copy link
Member

This is ready to be picked up again; note that we want to use 100 characters line length.

We've also agreed that we want to use an ignoreRevsFile for any commits to enforce black (whether in this PR or a follow-up one). On current master:

$ black --check -l 100 .
would reformat /home/erik/Code/securedrop-proxy/setup.py
would reformat /home/erik/Code/securedrop-proxy/securedrop_proxy/entrypoint.py
would reformat /home/erik/Code/securedrop-proxy/tests/test_entrypoint.py
would reformat /home/erik/Code/securedrop-proxy/securedrop_proxy/proxy.py
would reformat /home/erik/Code/securedrop-proxy/tests/test_proxy.py

To have latest black, we need updated typed-ast,
for that we have to upgrade mypy too, and for mypy
upgrade to work, we had to upgrade the mypy-extensions.

It first runs isort to check if it passes, and then
it runs black. Both uses 100 as line length.

setup.py marks Python version as >= 3.7 as we are testing
and running the code only on 3.7 on Debian Buster.

Also contains the formatting change in proxy.py for black
check to run sucessfully on CI.
It adds a configuration file to skip the previous commmit
which has isort and black formatting changes.

```
git config blame.ignoreRevsFile .git-blame-ignore-revs
````

After one executes the above command, `git blame` does not show
details for the formatting commit.
@kushaldas kushaldas force-pushed the run_black_run_in_ci_and_on_dev branch from 98193e3 to 32e6271 Compare June 4, 2020 13:10
@kushaldas kushaldas changed the title Fixes #57 runs black for code formatting check Fixes #57 runs black & isor for code formatting check Jun 4, 2020
@kushaldas
Copy link
Contributor Author

This is ready for review, the description has the new steps for testing.

@emkll emkll moved this from Sprint #52 - 6/3-6/17 to Ready for Review in SecureDrop Team Board Jun 4, 2020
Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

👍

One thing I tripped over was that isort's list of third-party packages is derived from the active virtualenv, so developers will need to ensure their editor or IDE is using the project-specific virtualenv, or they might get unexpected results from isort.

@rmol rmol merged commit f6c2f9a into master Jun 4, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Jun 4, 2020
@rmol rmol deleted the run_black_run_in_ci_and_on_dev branch June 4, 2020 15:31
@eloquence eloquence changed the title Fixes #57 runs black & isor for code formatting check Fixes #57 runs black & isort for code formatting check Jun 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[securedrop-proxy] black in dev env and CI
3 participants