Move linters configurations in pyproject.toml #1699

MVrachev · 2021-11-30T15:08:16Z

Description of the changes being introduced by the pull request:

In pr #1626 a pyproject.toml was added, which can be used to configure code formatters all of our
code formatters in one place only. It's good if we use one file as a source for truth for all linter
configurations.

I was able to move all linter configurations for pylint and mypy inside pyproject.toml,
but I faced difficulties with black and isort.
I explained what problems I faced moving some of the arguments in those comments:

black target folders: Configure black and isort only once in pyproject.toml #1684 (comment)
isort target folders: Configure black and isort only once in pyproject.toml #1684 (comment)
isort options --check and --dff: Configure black and isort only once in pyproject.toml #1684 (comment)

I decided to call black and isort without specifying the config file as they automatically find pyproject.toml
as it's at the root of the project.
It's not the same situation for pylint.

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

MVrachev · 2021-11-30T15:12:49Z

@lukpueh will appreciate a look from you as this is a TODO item added by you 9 months ago :D

coveralls · 2021-11-30T15:17:59Z

Pull Request Test Coverage Report for Build 1526231479

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 97.58%

Totals
Change from base Build 1520084662:	0.0%
Covered Lines:	4030
Relevant Lines:	4114

💛 - Coveralls

lukpueh

Thanks for working on this @MVrachev. I very briefly took a stab at it before writing #1684, but got too annoyed with the configuration issues you described. So, I all the more appreciate that you followed through! 👍

pyproject.toml

lukpueh · 2021-12-01T10:25:53Z

pyproject.toml

+  "tuf/exceptions.py"
+]
+
+[[tool.mypy.overrides]]


Why double brackets?

That was suggested by the mypy pyproject.toml configuration file documentation here.
I tried running it without one of the bracket pairs and got the error:

pyproject.toml: tool.mypy.overrides sections must be an array. Please make sure you are using double brackets like so: [[tool.mypy.overrides]]

lukpueh · 2021-12-01T10:33:14Z

pyproject.toml

+inlinevar-rgx="^[a-z][a-z0-9_]*$"
+module-rgx="^(_?[a-z][a-z0-9_]*|__init__)$"
+no-docstring-rgx="(__.*__|main|test.*|.*test|.*Test)$"
+variable-rgx="^[a-z][a-z0-9_]*$"


Why are some regexes in triple-quotes and others not? """rgx""" vs "rgx"?

A good catch. I copied the regex content from tuf/api/pylintrc and made the mistake of not reading it.
Fixing it.

But in tuf/api/pylintrc there are no quotes at all in these lines. (I actually compared the lines in both files while reviewing and the only difference are the quotes.)

Aaa, I think I wanted to split them multiline but decided against it.
No matter what was the reason it's fixed now.

lukpueh · 2021-12-01T10:42:50Z

tox.ini

-    pylint -j 0 tuf/api tuf/ngclient --rcfile=tuf/api/pylintrc
+    black tuf/api tuf/ngclient
+    isort --check --diff tuf/api tuf/ngclient
+    pylint -j 0 tuf/api tuf/ngclient --rcfile=pyproject.toml


Let's also format and lint the scripts in the newly added examples directory. (see #1685)

Will add a separate pr for that as I noticed there are a couple of mypy warnings that must be addressed as well.

Oh right, we already have a ticket for that in #1697

After we agree that this pr is good, I will submit a pr for #1697.

docs/CONTRIBUTORS.rst

This aims to add a single source of truth for pylint and mypy configurations. Signed-off-by: Martin Vrachev <[email protected]>

We are using 4 linters: black, isort, pylint and mypy. It's good if we use one file as a source for truth for all linter configurations. As a first step move black options in pyproject.toml. I tried multiple ways to use the include option, so we can just call black --config=pyproject.toml, but I was not successful. Then I found this comment psf/black#861 (comment) explaining that the path argument is mandatory. As a result, I will move all configuration options for black inside pyproject.toml without the actual directories that need to be linted. Signed-off-by: Martin Vrachev <[email protected]>

We are using 4 linters: black, isort, pylint and mypy. It's good if we use one file as a source for truth for all linter configurations. I tried multiple ways to use the src_path option, so we can just call isort without pointing out the target folders, but I was not successful. I tried running isort with "isort --settings-path=pyproject.toml" I got the error: "Error: arguments passed in without any paths or content." Additionally, I saw one project with source configuration https://github.com/Pylons/pyramid/blob/master/pyproject.toml, but they had to give explicit folders too https://github.com/Pylons/pyramid/blob/8061fce297cc7117d3e6e2b39e47512c7db2904f/tox.ini#L26 and https://github.com/Pylons/pyramid/blob/8061fce297cc7117d3e6e2b39e47512c7db2904f/tox.ini#L66 It was a similar situation with "check" and "diff". In the documentation it's said that for both check and diff are not supported in configuration files. See: - https://pycqa.github.io/isort/docs/configuration/options.html#check - https://pycqa.github.io/isort/docs/configuration/options.html#show-diff Additionally, in two issues it was confirmed that in integration tests we should use --check and --diff the way we did until now. As a result, I moved part of the configuration options for isort inside pyproject.toml without the actual directories that need to be linted and "check" and "diff" options. Signed-off-by: Martin Vrachev <[email protected]>

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev · 2021-12-01T14:29:22Z

I had to force-push because I linked the black configuration documentation in the build section instead of the black section.
No other changes were made.

jku

LGTM, with one suggestion that could be future work

jku · 2021-12-02T10:00:57Z

pyproject.toml

+files = [
+  "tuf/api/",
+  "tuf/ngclient",
+  "tuf/exceptions.py"
+]


these make sense here so you can just run mypy... but I wonder if maintenance would be easier if we gave these on the command line in tox.ini like all other tools -- then we could define a lint_dirs variable in there once and use it for all tools (mypy would need a exceptions.py added until we move the file to api/ but that's fine)

Generally a good idea! But let's hold off this future work until we've removed legacy code, shall we? Maybe we won't need a lint_dirs then.

coveralls · 2024-12-26T15:03:22Z

Pull Request Test Coverage Report for Build 1526231479

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.0%) to 98.579%

Totals
Change from base Build 1520084662:	1.0%
Covered Lines:	3869
Relevant Lines:	3895

💛 - Coveralls

MVrachev requested a review from lukpueh November 30, 2021 15:08

lukpueh requested changes Dec 1, 2021

View reviewed changes

Move mypy and pylint configs in pyproject.toml

8ba3cc4

This aims to add a single source of truth for pylint and mypy configurations. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the move-configs branch from 2442cff to 4b2f227 Compare December 1, 2021 14:15

MVrachev requested a review from lukpueh December 1, 2021 14:18

MVrachev mentioned this pull request Dec 1, 2021

examples: run mypy on repo example #1697

Closed

lukpueh approved these changes Dec 1, 2021

View reviewed changes

MVrachev added 3 commits December 1, 2021 16:28

Fix small pylint error

5c8a866

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the move-configs branch from 4b2f227 to 5c8a866 Compare December 1, 2021 14:28

MVrachev mentioned this pull request Dec 1, 2021

Start linting the examples folder #1701

Merged

3 tasks

jku approved these changes Dec 2, 2021

View reviewed changes

jku merged commit d991362 into theupdateframework:develop Dec 2, 2021

MVrachev deleted the move-configs branch December 10, 2021 12:00

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move linters configurations in pyproject.toml #1699

Move linters configurations in pyproject.toml #1699

MVrachev commented Nov 30, 2021 •

edited

Loading

MVrachev commented Nov 30, 2021

coveralls commented Nov 30, 2021 •

edited

Loading

lukpueh left a comment

lukpueh Dec 1, 2021

MVrachev Dec 1, 2021

lukpueh Dec 1, 2021

MVrachev Dec 1, 2021

lukpueh Dec 1, 2021

MVrachev Dec 1, 2021

lukpueh Dec 1, 2021

MVrachev Dec 1, 2021

lukpueh Dec 1, 2021

MVrachev Dec 1, 2021 •

edited

Loading

MVrachev commented Dec 1, 2021 •

edited

Loading

jku left a comment

jku Dec 2, 2021

lukpueh Dec 2, 2021

coveralls commented Dec 26, 2024 •

edited

Loading

Move linters configurations in pyproject.toml #1699

Move linters configurations in pyproject.toml #1699

Conversation

MVrachev commented Nov 30, 2021 • edited Loading

MVrachev commented Nov 30, 2021

coveralls commented Nov 30, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1526231479

💛 - Coveralls

lukpueh 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

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

MVrachev Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

MVrachev commented Dec 1, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 26, 2024 • edited Loading

Pull Request Test Coverage Report for Build 1526231479

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

MVrachev commented Nov 30, 2021 •

edited

Loading

coveralls commented Nov 30, 2021 •

edited

Loading

MVrachev Dec 1, 2021 •

edited

Loading

MVrachev commented Dec 1, 2021 •

edited

Loading

coveralls commented Dec 26, 2024 •

edited

Loading