Update linter and add auto-formatter config #1314

lukpueh · 2021-03-12T19:52:11Z

Description of the changes being introduced by the pull request:

Configure stricter linting, and enable and require auto-formatting to reduce future reviewing efforts for new code as per ADR3 in tuf/api/*. More specifically, this PR:

updates tuf/api/pylintrc,
configures tox to require code to be formatted by black (overall code style) and isort (order of import statements),
adds pre-commit configuration, which, upon optional local enabling, automatically runs black, isort and two basic builtin formatters on each git commit.

This PR further updates tuf/api/* source code using the added auto-formatters.
(If desired I can split commits by change and/or configure git for this repo to ignore the corresponding revision(s) in git-blame).

See commit messages for details.

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

Use black and isort to reformat new code in tuf/api/*, like so: ``` black --line-length 80 api isort --line-length 80 --profile black api ``` Besides downsizing the default line length to fit our Code Style Guide no extra configuration is required. Unified format according to black and isort will be enforced by CI/CD in a future commit. **Changes include:** - Use double quotes instead of single quotes where feasible - Re-wrap and re-indent long lines such as dict literals, function signatures and function calls, using hanging indent This will require an update in our Code Style Guide, which the benefits of using black seem worth. https://github.com/secure-systems-lab/code-style-guidelines/blob/master/python.md#indentation-and-line-continuation - Update vertical and horizontal spacing - Sort and wrap imports See black and isort docs for details: https://black.readthedocs.io/en/stable/the_black_code_style.html https://pycqa.github.io/isort/docs/configuration/black_compatibility/ NOTE: If desired I can split commits by change and/or configure git for this repo to ignore the corresponding revision(s) in git-blame. https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame Signed-off-by: Lukas Puehringer <[email protected]>

The updated pylintrc is based on the Google Python Style Guide pylint configuration at https://google.github.io/styleguide/pylintrc with the following differences: - We don't list defaults which are applied anyway. - We don't configure checks that seem unrelated to the code style guide. - We don't disable any checks that are not in conflict with the current code or code style guide. This has the advantage of a minimal configuration file which should be easy to maintain and extend as required, e.g. if conflicting code is added, or linting time becomes too long, due to unnecessary checks. Signed-off-by: Lukas Puehringer <[email protected]>

Configure lint build in tox.ini to check if code in tuf/api/* is formatted according to black and isort style rules: https://black.readthedocs.io/en/stable/the_black_code_style.html https://pycqa.github.io/isort/ In addition to our new style guide (theupdateframework#1128) and corresponding linter configuration, requiring auto-formatting should help to further reduce reviewing effort. The auto-formatter black was chosen for the following reasons: - It seems to be the most popular formatter in the Python ecosystem - It is well documented including integration instructions with most of the tools we use (git, GitHub Actions, pylint, a range of editors, pyproject.toml theupdateframework#1161) - It checks that the reformatted code produces a valid AST that is equivalent to the original - It has almost no ways of customization, which means no customization effort required, and more (cross-project) style uniformity, lowering contribution barriers - It converts single to double quotes, where reasonable, which is exactly what we recommend - The style choices it makes seem generally reasonable and don't conflict with our style guide, except for favoring hanging over aligned indentation, which is the opposite of what we recommend. But we are willing to update the adapt our style guide. Auto-format pre-commit configuration will be added in a subsequent commit. Signed-off-by: Lukas Puehringer <[email protected]>

Add optional pre-commit configuration to install and run auto-formatters when committing new code to tuf/api/*. Auto-formatters include: - trailing-whitespace - end-of-file-fixer - black - isort This commit also adds pre-commit to the dev dependencies and updates the contributor instructions accordingly. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh · 2021-03-15T08:27:41Z

.pre-commit-config.yaml

+files: ^tuf/api/
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v3.4.0


We should consider auto-updating the hooks in this file. Unfortunately, dependabot does not plan to support this (see dependabot/dependabot-core#1524). But there's a Github Action based workaround. Or we could use pre-commit's own ci -- pre-commit.ci. Although I lean towards not adding another CI/CD platform.

MVrachev

I agree with using black as a formatter.
Your arguments make sense.

We would invoke black with:
black --line-length 80 api right?
Because we use 80 characters per line instead of 88.

MVrachev · 2021-03-15T13:03:23Z

docs/CONTRIBUTORS.rst

+and `isort <https://pycqa.github.io/isort>`_. The tasks can be installed as
+`pre-commit <https://pre-commit.com/>`_ git hooks with the following command.
+::
+    $ pre-commit install


When you clone tuf you will have the .pre-commit-config.yaml locally right?
Then, what does this command do exactly?

lukpueh · 2021-03-16T08:56:58Z

Thanks for the review, @MVrachev! Regarding your comments/questions...

I agree with using black as a formatter. Your arguments make sense.

🎉

When you clone tuf you will have the .pre-commit-config.yaml locally right?

Yes!

Then, what does this command [pre-commit install] do exactly?

It installs a script to a local .git/hooks/pre-commit file which will run the hooks (black, isort, etc.) specified in .pre-commit-config.yaml whenever you run git commit.

I recommend taking a quick look at the pre-commit documentation it's really a cool tool built on top of git.

We would invoke black with: black --line-length 80 api right? Because we use 80 characters per line instead of 88.

Yes, black needs to be invoked with --line-length 80, as you can see in tox.ini and .pre-commit-config.yaml. Once we have a pyproject.toml file (see #1161) we can configure this there for all uses of black in the project, be it tox, pre-commit, or you on the command line.

Also note that there's no reason to run black from the command line if you install the pre-commit hook.

MVrachev · 2021-03-16T11:16:10Z

I would suggest adding documentation on how to run black from the command line as an addition to the git pre-commit hooks.

Sometimes you would want to run black through your command line before you commit your changes.

joshuagl

This is great, thanks @lukpueh. I am looking forward to not worrying about coding style any more.I fully agree that changing our coding style (hanging indents) to match black's style is the right choice, both because this gives contributors a more consistent style across Python projects, and because it minimises how much attention contributors have to pay to formatting (by automating it).

The one piece of this PR I am less enthusiastic about is providing a pre-commit configuration. I'm not completely against it, but it's one extra thing for us to maintain and something we have to tell contributors to enable. I wonder whether we might be better off updating docs/CONTRIBUTORS.rst to recommend contributors configure their editor to use black (linking to the black docs) or use pre-commit (and include a snippet like the .pre-commit-config.yaml in this PR) and then not include the file in our repository (avoiding the need for us to automate keeping it up-to-date).

joshuagl · 2021-03-17T09:39:19Z

tuf/api/metadata.py

        deserializer: Optional[MetadataDeserializer] = None,
-        storage_backend: Optional[StorageBackendInterface] = None
-    ) -> 'Metadata':
+        storage_backend: Optional[StorageBackendInterface] = None,


Woah, trailing commas in function signatures caught me off guard 🤯
The rationale in PEP 8, and black's documentation, seems reasonable. I am glad we will have a tool to automatically do this, though.

joshuagl · 2021-03-17T09:46:18Z

tuf/api/metadata.py

-                    f'{len(signatures_for_keyid)} signatures for key '
-                    f'{key["keyid"]}, not sure which one to verify.')
+                f"{len(signatures_for_keyid)} signatures for key "
+                f'{key["keyid"]}, not sure which one to verify.'


This f-string (and the one above on ln269) looks a bit out of place, I wonder whether we should manually change these to f"dict['key']" - would black change those back?

Well spotted! I missed this when I prepared the PR. Looks like black doesn't change double quotes to single quotes nor adds escape characters, as a consequence it can't change the outer single quotes to double quotes. Unfortunately, pylint's quote consistency check also doesn't detect this, so I fear the onus will remain on the reviewer here. At least black does not change it back.

Fixed in be0cef0.

Black standardizes single to double quotes where feasible. However, it doesn't seem to change double to single quotes nor adds escape characters, as a consequence it skips standardization on strings with mixed quotes. Unfortunately, pylint's quote consistency check also doesn't detect this, so the onus will remain on the reviewer in these cases. **Unrelated changes**: The commit still enables pylint's "check-quote-consistency" just in case it can detect something the black doesn't. The commit also fixes a syntax inconsistency in pylintrc. Signed-off-by: Lukas Puehringer <[email protected]>

This reverts commit "Add basic pre-commit configuration for tuf/api/*" (44aea45) in order to reduce maintenance burdern: - pre-commit really is a package manager, thus the packages (git hooks) pulled in via pre-commit would need to be kept up-to-date and securely so (sic!). - pre-commit requires contributors to opt-in via "pre-commit install" regardless, so we might as well ask contributors to add and tend to the corresponding configuration file on their own. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh · 2021-03-17T13:05:08Z

Ping @MVrachev and @joshuagl, this is ready for a re-review, whenever you have time! 🙏

lukpueh · 2021-03-17T13:07:03Z

I would suggest adding documentation on how to run black from the command line as an addition to the git pre-commit hooks.

Fixed in 2e883d4

lukpueh · 2021-03-17T13:08:05Z

The one piece of this PR I am less enthusiastic about is providing a pre-commit configuration. I'm not completely against it, but it's one extra thing for us to maintain and something we have to tell contributors to enable. I wonder whether we might be better off updating docs/CONTRIBUTORS.rst to recommend contributors configure their editor to use black (linking to the black docs) or use pre-commit (and include a snippet like the .pre-commit-config.yaml in this PR) and then not include the file in our repository (avoiding the need for us to automate keeping it up-to-date).

Fixed in 38ef45f.

joshuagl · 2021-03-17T13:11:32Z

docs/CONTRIBUTORS.rst

+or via source code editor plugin
+[`black <https://black.readthedocs.io/en/stable/editor_integration.html>`__,
+`isort <https://github.com/pycqa/isort/wiki/isort-Plugins>`__] or
+`pre-commit <https://pre-commit.com/>`__-powerd git hooks


Suggested change

`pre-commit <https://pre-commit.com/>`__-powerd git hooks

`pre-commit <https://pre-commit.com/>`__-powered git hooks

Fixed and force-pushed!

Add cli snippet to run black and isort on the command line and pointers to editor and pre-commit configuration to docs/CONTRIBUTORS.rst. Also add .pre-commit-config.yaml to .gitignore for independent pre-commit configuration. Signed-off-by: Lukas Puehringer <[email protected]>

joshuagl

This LGTM, thanks @lukpueh!

I will hold off on merging to give @MVrachev a chance to re-review.

lukpueh · 2021-03-18T09:38:46Z

Let's also land #1261 first for easier conflict resolution.

MVrachev

This LGTM, thanks @lukpueh!

I will hold off on merging to give @MVrachev a chance to re-review.

Thank you, Joshua!
LGTM!

lukpueh · 2021-03-19T14:40:12Z

Thanks again for the reviews, @joshuagl and @MVrachev! Merging...

lukpueh · 2021-03-19T14:41:12Z

Oh no. Silly me. Forgot about #1261. 🤦

lukpueh · 2021-03-19T14:41:57Z

@jku, I'm happy to help rebasing.

jku · 2021-03-19T14:46:05Z

Sorry I've been distracted: thanks for the offer but I'm just about to do the rebase now

lukpueh added 4 commits March 12, 2021 11:51

lukpueh commented Mar 15, 2021

View reviewed changes

MVrachev approved these changes Mar 15, 2021

View reviewed changes

joshuagl reviewed Mar 17, 2021

View reviewed changes

lukpueh added 2 commits March 17, 2021 11:57

lukpueh mentioned this pull request Mar 17, 2021

api: Lint metadata.py #1294

Closed

3 tasks

joshuagl reviewed Mar 17, 2021

View reviewed changes

lukpueh force-pushed the update-api-linting branch from 2e883d4 to f9bf52f Compare March 17, 2021 13:13

joshuagl approved these changes Mar 18, 2021

View reviewed changes

lukpueh mentioned this pull request Mar 19, 2021

WIP: Client refactor #1291

Merged

17 tasks

MVrachev approved these changes Mar 19, 2021

View reviewed changes

lukpueh merged commit c2b1f0e into theupdateframework:develop Mar 19, 2021

lukpueh mentioned this pull request Feb 17, 2022

Revise pylint configuration (and linter choice) #1178

Closed

lukpueh mentioned this pull request May 22, 2023

lint: add pre-commit config secure-systems-lab/securesystemslib#582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update linter and add auto-formatter config #1314

Update linter and add auto-formatter config #1314

lukpueh commented Mar 12, 2021

lukpueh Mar 15, 2021

MVrachev left a comment

MVrachev Mar 15, 2021

lukpueh commented Mar 16, 2021

MVrachev commented Mar 16, 2021 •

edited

Loading

joshuagl left a comment

joshuagl Mar 17, 2021

joshuagl Mar 17, 2021

lukpueh Mar 17, 2021

lukpueh Mar 17, 2021

lukpueh commented Mar 17, 2021

lukpueh commented Mar 17, 2021 •

edited

Loading

lukpueh commented Mar 17, 2021

joshuagl Mar 17, 2021

lukpueh Mar 17, 2021

joshuagl left a comment

lukpueh commented Mar 18, 2021

MVrachev left a comment

lukpueh commented Mar 19, 2021

lukpueh commented Mar 19, 2021

lukpueh commented Mar 19, 2021

jku commented Mar 19, 2021

	`pre-commit <https://pre-commit.com/>`__-powerd git hooks
	`pre-commit <https://pre-commit.com/>`__-powered git hooks

Update linter and add auto-formatter config #1314

Update linter and add auto-formatter config #1314

Conversation

lukpueh commented Mar 12, 2021

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Mar 16, 2021

MVrachev commented Mar 16, 2021 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Mar 17, 2021

lukpueh commented Mar 17, 2021 • edited Loading

lukpueh commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl left a comment

Choose a reason for hiding this comment

lukpueh commented Mar 18, 2021

MVrachev left a comment

Choose a reason for hiding this comment

lukpueh commented Mar 19, 2021

lukpueh commented Mar 19, 2021

lukpueh commented Mar 19, 2021

jku commented Mar 19, 2021

MVrachev commented Mar 16, 2021 •

edited

Loading

lukpueh commented Mar 17, 2021 •

edited

Loading