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

Add linting via ruff #1059

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add linting via ruff #1059

wants to merge 15 commits into from

Conversation

dosisod
Copy link
Contributor

@dosisod dosisod commented Sep 15, 2023

Ruff is a really great flake8 alternative that includes a very comprehensive set of linter rules. This PR doesn't remove flake8, but since Ruff is meant to be a replacement of flake8 (and implements a superset of the same rules, click here for a full list), we could drop flake8 entirely.

I've configured Ruff to enable all available linter rules, and manually disabled all the ones that don't apply or are too noisy. Since Ruff is actively under development, any new lint rules are likely to be overlooked. By enabling them all (and fixing any errors), it is much easier to keep the codebase up to date.

Each commit is broken down to a single lint rule, or a group of similar lint rules. Feel free to yay or nay the commits you do or don't like, I can easily rebase accordingly.

Probably closes #874 as this PR converts format() calls to fstrings.

@dosisod dosisod requested a review from a team as a code owner September 15, 2023 05:06
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #1059 (ef00d8e) into main (356b3d8) will decrease coverage by 0.19%.
The diff coverage is 65.00%.

@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
- Coverage   85.02%   84.84%   -0.19%     
==========================================
  Files          65       65              
  Lines        3139     3095      -44     
==========================================
- Hits         2669     2626      -43     
+ Misses        470      469       -1     
Files Changed Coverage Δ
hvac/api/auth_methods/aws.py 57.05% <0.00%> (-0.28%) ⬇️
hvac/api/auth_methods/azure.py 91.30% <ø> (-0.19%) ⬇️
hvac/api/auth_methods/gcp.py 89.61% <0.00%> (-0.14%) ⬇️
hvac/api/auth_methods/github.py 100.00% <ø> (ø)
hvac/api/auth_methods/jwt.py 92.10% <ø> (-0.21%) ⬇️
hvac/api/auth_methods/kubernetes.py 91.48% <0.00%> (+1.69%) ⬆️
hvac/api/auth_methods/legacy_mfa.py 33.33% <ø> (-2.16%) ⬇️
hvac/api/auth_methods/mfa.py 100.00% <ø> (ø)
hvac/api/auth_methods/oidc.py 100.00% <ø> (ø)
hvac/api/auth_methods/okta.py 100.00% <ø> (ø)
... and 39 more

@briantist briantist self-assigned this Sep 16, 2023
@briantist briantist added dependencies Pull requests that update a dependency file maintenance General technical debt developer experience Developer setup and experience labels Sep 16, 2023
@briantist
Copy link
Contributor

Hello again @dosisod ! First I want to say thanks for the detailed explanation, and especially for splitting the changes per-rule into discrete commits, that's exactly how I would've liked to see that 🤩

Some of the changes it made look really nice and are definitely ones I agree with, some of the others, I'm a little less sure about or don't understand.

I am going to leave this open but I will have to deprioritize this change a little bit until I can give it more thought (especially per-rule) and try to discuss with the other maintainers. I definitely intend to re-evaluate it, but likely at some point post-v2.0.0, hope that's ok!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file developer experience Developer setup and experience maintenance General technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants