chore: Update linters and essential configurations #743
Conversation
for more information, see https://pre-commit.ci
|
@webknjaz is there a way to make As it generates changes for all these files, when none of them were actually touched in this PR |
This is actually good — new linters should include changes to the linted things. This makes such changes atomic. |
| types_or: [python, pyi] | ||
| - id: ruff-format | ||
| types_or: [python, pyi] | ||
| args: [--config, format.quote-style = 'single', --config, line-length = 100] |
There was a problem hiding this comment.
There should be a good reason(s) to deviate from linter's default settings.
There was a problem hiding this comment.
It's not because of the defaults (for which sometimes there are good reasons to change) but because other tools wouldn't pick this up, it should be avoided (with a few well-defined exceptions like the one I've got for MyPy).
For this reason, every low-level tool must be configured via its own autoloaded config file name. Otherwise, everybody would have to duplicate this config in their editors, IDEs, other tools and sometimes CI, and would have toe know upfront that this is necessary. Being surprised by unexpected behaviors when they don't. I have some notes on this in #742.
I wouldn't say that setting such a long line setting has a merit, though. I'm a firm believer that in the name of readability/inclusivity/maintainability, code should be readable in columns. Just like any text, typography can govern column width for code too. This is directly connected to readability (which is why magazines and newspapers don't print lines from one side of the page to the others, and some books / catalogs use columns). Typographically optimal columns are between 50 and 75 chars (opinions vary, but mostly close to this range): https://baymard.com/blog/line-length-readability. We have a long-standing convention of 79 that exceeds it a bit, but isn't as critical. It allows for columns in editors for working on multiple files while still using enlarged fonts. It also allows for side-by-side diffs. The lines fit on the screen in these settings as well as allow for column-based top-down reading (which is another thing humans do with text naturally, by habit).
| args: | ||
| - -i | ||
| - --max-line-length=100 |
There was a problem hiding this comment.
Out of interest: why do you sometimes use args: […] and sometimes split args arrays to multiline? Such inconsistency is a bit confusing as it adds impression that this is done for a reason.
There was a problem hiding this comment.
sometimes it depends on how hook configured - IE
[--config, format.quote-style = 'single', --config, line-length = 100]will be actually more readble as
[
--config, format.quote-style = 'single',
--config, line-length = 100
]not
- --config
- format.quote-style = 'single'
- --config
- line-length = 100But I don't remember why I can't use multiline array with [], maybe I just not set it in first place when set it a long time ago.
Second reason - our God of code reusage across projects - copy-paste :D
Nobody never reevaluated it, so it is as it is till now
There was a problem hiding this comment.
This is rather subjective. I'd argue that [] is the least readable way.
Actually, for --long-args, it's best to use = not a whitespace. This is useful not only in this config but in Python code too. Since you're integrating autoformatters, they will produce such weird constructs with args disconnected from their values unless you use equals that bundles them into the same string.
| hooks: | ||
| - id: mypy | ||
| additional_dependencies: | ||
| - types-PyYAML |
There was a problem hiding this comment.
Why four rather than two spaces indentation at this line? 😕
There was a problem hiding this comment.
yamllint all the things!
I'd add https://github.com/ansible/awx-plugins/blob/53ebc59/.yamllint with quoted-strings: {required: only-when-needed} on top (https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.quoted_strings).
|
|
||
|
|
||
| ########## | ||
| # PYTHON # |
There was a problem hiding this comment.
With pre-commit, the primary ordering should be by whether something is faster or slower. Putting instant checks at the beginning improves responsiveness, with the slow ones stuffed at the end. The second factor is formatters vs. linters. When a bunch of formatters change some type of files, they should be executed before the linters that check the same files.
The ecosystem is something that could be used to bundle similar checks within those groups. But it's definitely not something that would be top level.
| hooks: | ||
| - id: pylint | ||
| args: | ||
| - --disable=import-error # E0401. Locally you could not have all imports. |
There was a problem hiding this comment.
Avoid putting linter settings into CLI args at all costs. This is incompatible with literally everything that would run said linters. Always keep them in tool-specific configs.
| - --disable=fixme # W0511. 'TODO' notations. | ||
| - --disable=logging-fstring-interpolation # Conflict with "use a single formatting" WPS323 | ||
| - --disable=ungrouped-imports # ignore `if TYPE_CHECKING` case. Other do reorder-python-imports | ||
| - --disable=R0801 # Similar lines in 2 files. Currently I don't think that it possible to DRY hooks unique files boilerplate |
There was a problem hiding this comment.
Don't use numeric rule codes in pylint. It has human-readable names that make much more sense than random digit sequences.
Also, instead of disabling the rules, many pylint and flake8 rules can remain enabled because they allow tweaking their settings. I know for a fact that this duplication rule allows increasing the number of lines to take into account.
There was a problem hiding this comment.
👍
I can only pray that pylint maintainers will add human-readable names in their error msgs, and not require users to google if they exist and how they named if so
There was a problem hiding this comment.
I have this configured by default 🤷♂️ You should've just used my config.
There was a problem hiding this comment.
Unfortunately, Ruff does not allow using those names in ignores, while the rules may have human-readable names. There's a feature request about it.
| args: | ||
| - --disable=import-error # E0401. Locally you could not have all imports. | ||
| - --disable=fixme # W0511. 'TODO' notations. | ||
| - --disable=logging-fstring-interpolation # Conflict with "use a single formatting" WPS323 |
There was a problem hiding this comment.
Drop this. Logging actually shouldn't be using f-strings.
| hooks: | ||
| - id: mypy | ||
| additional_dependencies: | ||
| - types-PyYAML |
There was a problem hiding this comment.
yamllint all the things!
I'd add https://github.com/ansible/awx-plugins/blob/53ebc59/.yamllint with quoted-strings: {required: only-when-needed} on top (https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.quoted_strings).
| --ignore-missing-imports, | ||
| --disallow-untyped-calls, | ||
| --warn-redundant-casts, |
There was a problem hiding this comment.
Nothing should be disabled globally. Suppressions must be granular, which can be implemented in the config and in-module.
| - --max-imports=15 # Default to 12 | ||
| # https://wemake-python-stylegui.de/en/latest/pages/usage/violations/index.html | ||
| - --extend-ignore= | ||
| E501 <!-- line too long (> 79 characters). Use 100 --> |
There was a problem hiding this comment.
Configurable rules shouldn't be ignored really.
There was a problem hiding this comment.
- It covered by other linters, so deduplicate similar errors
- Idk why it not provide link to docs which describes possibilities how to deal with violation as many hooks do 🤔 It's not easy to google them. This one https://www.flake8rules.com/rules/E501.html doesn't include any suggestions about configuration -_-
There was a problem hiding this comment.
WPS can show shortlinks to the rules, you just didn't enable it 🤷♂️
This is the setting: https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-max-line-length.
But anyway, it doesn't make sense to have such things as CLI args. I can send in a good config structure where it's more apparent what should go where.
| 'Programming Language :: Python :: 2', | ||
| 'Programming Language :: Python :: 2.7', | ||
| 'Programming Language :: Python :: 3', | ||
| 'Programming Language :: Python :: 3.6', | ||
| 'Programming Language :: Python :: 3.7', |
There was a problem hiding this comment.
It's best to list envs tested in CI here.
There was a problem hiding this comment.
It's usually best to couple formatting changes with adding a formatter. Not multiple automatic and manual formatting changes with formatters and linters all smashed together. It's difficult to figure out what's coming from where and why that is in this setting.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com> Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
| - --max-line-length=100 | ||
|
|
||
| # Usage: http://pylint.pycqa.org/en/latest/user_guide/message-control.html | ||
| - repo: https://github.com/PyCQA/pylint |
There was a problem hiding this comment.
I experimented with configuring this linter for a while, but I'm hitting a few bugs within.
You're also integrating Ruff which reimplements most if not all of pylint rules in it.
It doesn't really make sense to have multiple linters report the same violation multiple times within each pre-commit run.
With that in mind, I recommend abandoning pylint for now and focusing on making sure all the corresponding rules in Ruff are enabled instead.
There was a problem hiding this comment.
Yeah, I guess it's a worthwhile suggestion.
There was a problem hiding this comment.
FWIW, I'm planning to research replacing many things with Ruff in my projects but just didn't get to do a full comparison.
There was a problem hiding this comment.
Hmm, ruff has rules that should be manually enabled? Didn't know it
From what I saw - pylint in its current configuration provides additional checks which are not covered by ruff in its current configuration.
At the same time, it totally makes sense to check is ruff and we-make-styleguide both fully cover pylint out of the box or at least able to be configured in such way.
If you do such research - that's would be lovely, but till I want to have all them in place, as better to catch all possible issues now, than deal with them in 6-12months when they detection will be implemented in ruff etc.
pylint check is relatively fast, so I want to preserve it for now
There was a problem hiding this comment.
pylintcheck is relatively fast
😆 this is the slowest checker I know. This is because it goes beyond static analysis and does some dynamic checking as well.
As for ruff, I wanted to research it more. And yes, there's rules that are disabled by default in all the linters I've ever met (flake8, pylint, ruff). Some updates to pylint sometimes move the rules from default to extensions too — this is why I prefer an explicit config to make sure that it doesn't suddenly stop checking something just because of a version bump.
While I do like pylint in general, I tried it locally on this codebase and there's bugs in it that block really adding it in full capacity. Hence, the idea to delay adding it. I may end up debugging the issue for my other projects. But in general, many people stick with flake8 because it's not as slow and easier to extend. We do run it in ansible-test, though.
My initial strategy for adopting Ruff would be enabling as much as possible there, and only if some rule is not ported, then run flake8/pylint (with plugins) only checking those rules, to reduce the overhead. It's good for DX when the same problem isn't reported by 5 different checkers.
| - repo: https://github.com/pre-commit/mirrors-autopep8 | ||
| rev: v2.0.4 | ||
| hooks: | ||
| - id: autopep8 |
There was a problem hiding this comment.
With Ruff in formatting mode, this is probably unnecessary. I'm yet to compare their output, but the recommendation is going to be the same as with pylint for now.
| name: isort | ||
| args: [--force-single-line, --profile=black] | ||
|
|
||
| - repo: https://github.com/asottile/add-trailing-comma |
There was a problem hiding this comment.
This one might be fine to combine with Ruff. But you'll have to put it first and also check if they don't cancel each other out in the way they're configured.
There was a problem hiding this comment.
They don't
I wouldn't be so sure. I've seen corner cases with various formatters where they'd do this very occasionally under certain conditions. It's good that we haven't observed such a behavior here. I'm just saying that this should be kept in mind for the future in case it does happen.
If Ruff does not implement a similar capability, it stands to reason that it's okay to keep this formatter. If it does, it might not make sense.
But if you decide to keep this one, make sure to move it along with any other small narrow-scoped formatters before the Ruff run.
|
This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 10 days |
github-actions
bot
added
the
stale
WalkthroughThis pull request updates several configuration and source-code files. The GitHub Actions workflow now uses Python 3.10 instead of 3.9. New entries have been added to .gitignore, and the pre-commit configuration has been expanded with additional Python linting and formatting hooks. A new VSCode setting is introduced to include ./src in Python analysis. In pyproject.toml, outdated Python version classifiers are removed and the project description is updated. Several source files have been reformatted for clarity, including adjustments to import statements, indentation, and method signatures, and one unused variable has been removed. Changes
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
105-116: Pylint Configuration Review.
Pylint is configured with several disable flags (e.g.--disable=import-error,--disable=fixme). While this helps reduce noise, please verify that none of the disabled warnings obscure valuable issues. A brief comment or documentation note explaining the rationale behind each disabled warning could be beneficial for future maintainers.
129-150: Wemake Python Styleguide Configuration.
The configuration for the wemake-python-styleguide hook—including increased limits for local variables, returns, and imports—is well tailored to your needs. Just ensure the extended ignore rules and exclusion patterns truly match your project's coding style without inadvertently masking issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/pre-commit.yaml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(2 hunks).vscode/settings.json(1 hunks)pyproject.toml(1 hunks)src/pre_commit_terraform/__main__.py(1 hunks)src/pre_commit_terraform/_cli.py(2 hunks)src/pre_commit_terraform/_cli_subcommands.py(0 hunks)src/pre_commit_terraform/_errors.py(1 hunks)src/pre_commit_terraform/_types.py(2 hunks)src/pre_commit_terraform/terraform_docs_replace.py(4 hunks)
💤 Files with no reviewable changes (1)
- src/pre_commit_terraform/_cli_subcommands.py
✅ Files skipped from review due to trivial changes (6)
- src/pre_commit_terraform/_errors.py
- .gitignore
- src/pre_commit_terraform/main.py
- src/pre_commit_terraform/terraform_docs_replace.py
- src/pre_commit_terraform/_cli.py
- src/pre_commit_terraform/_types.py
🔇 Additional comments (10)
.vscode/settings.json (1)
10-12: Expand Python Analysis Search Paths.
The new addition"python.analysis.extraPaths": ["./src"]is a useful enhancement for module resolution. Note that a previous comment suggested including the tests directory. If your tests contain analyzable modules, consider adding"./tests"as well.pyproject.toml (2)
12-13: Modernize Python Classifiers.
Replacing older classifiers with the new"Programming Language :: Python :: 3 :: Only"clearly signals that only Python 3 is supported. This modernization aligns with the upcoming tooling and documentation updates.
17-17: Update Project Description.
The project description has been updated to"Pre-commit hooks for Terraform", which better reflects the project’s focus. Please ensure this is consistently reflected in all associated documentation..github/workflows/pre-commit.yaml (1)
41-41: Upgrade Python Version in Workflow.
Changing the Python version from 3.9 to 3.10 in the setup step is a straightforward improvement that ensures compatibility with the latest tools and language features..pre-commit-config.yaml (6)
47-60: Hadolint Revision Update.
The hadolint hook's revision has been upgraded to"v2.13.1-beta". This should provide access to improved linting rules and bug fixes. Please verify that the updated version works as expected with your Dockerfile linting requirements.
75-84: Ruff & Ruff-Format Hook Configuration.
Integrating Ruff and the ruff-format hooks strengthens the Python linting setup. However, verify that the argument syntax used—particularly in
args: [--config, format.quote-style = 'single', --config, line-length = 100]—is parsed correctly by Ruff. It may be necessary to remove spaces around the equals sign (e.g.--config=format.quote-style=single) so that the arguments are interpreted properly.
85-91: Isort Integration.
The isort hook is configured with parameters like--force-single-lineand the Black profile, which should help maintain consistent import formatting.
92-96: Add Trailing Comma Hook.
Including the add-trailing-comma hook helps enforce a consistent style across your Python files. The configuration appears correct.
97-104: Autopep8 Formatting.
The autopep8 hook is set to enforce a maximum line length of 100, which is consistent with your other formatter configurations.
117-128: Mypy Type Checking Enhancements.
The mypy hook now includes additional dependencies (such astypes-PyYAML) and stricter type checking arguments. This is a positive step toward enhancing code quality through type safety.
github-actions
bot
removed
the
stale
Description of your changes
Subset of #741, to merge before #742 to prevent future violation to be merged, which will requires additional fixes after.
pyproject.tomlas this project never support Python 2Summary by CodeRabbit
New Features
Chores
Refactor/Style