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

Use ruff for linting (instead of current flake8/isort/pyflakes) #727

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

jairhenrique
Copy link
Collaborator

@jairhenrique jairhenrique commented Jun 26, 2023

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #727 (b49bea7) into master (34d5384) will increase coverage by 0.05%.
The diff coverage is 90.90%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
+ Coverage   90.59%   90.64%   +0.05%     
==========================================
  Files          28       28              
  Lines        1797     1796       -1     
  Branches      270      323      +53     
==========================================
  Hits         1628     1628              
+ Misses        134      133       -1     
  Partials       35       35              
Impacted Files Coverage Δ
vcr/cassette.py 96.31% <ø> (ø)
vcr/config.py 95.06% <ø> (ø)
vcr/request.py 97.53% <ø> (ø)
vcr/serialize.py 100.00% <ø> (ø)
vcr/stubs/__init__.py 93.87% <ø> (ø)
vcr/stubs/tornado_stubs.py 0.00% <ø> (ø)
vcr/_handle_coroutine.py 33.33% <50.00%> (ø)
vcr/__init__.py 100.00% <100.00%> (ø)
vcr/errors.py 100.00% <100.00%> (ø)
vcr/migration.py 74.48% <100.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi @jairhenrique I am wondering:

  • whether ruff should be replacing flake8 (rather than adding to it, the two living side by side) when https://beta.ruff.rs/docs/faq/#how-does-ruff-compare-to-flake8 says that only a "subset of the E and W rules (which originate from pycodestyle)" are implemented
  • whether it's a gain or a loss to drop the noqa markers, in particular those with a number.

What do you think?

@jairhenrique
Copy link
Collaborator Author

@hartwork I will enable default linters "E","F","W" to be more close with flake8.
I think the noqa markers will come back when enabling these linters.

@jairhenrique
Copy link
Collaborator Author

  • whether it's a gain or a loss to drop the noqa markers, in particular those with a number.

I usually don't use the numbers, I can't tell if this is a loss or gain.

@hartwork
Copy link
Collaborator

@jairhenrique what the numbers allow in theory is to annotate why something was marked noqa semantically (i.e. documentation) but also to limit the scope of the noqa marker to just that aspect so that the QA tool won't silence unrelated issues now or in the future by accident.

I wonder, if we drop the markers, why doesn't ruff complain about the related cases, why doesn't ruff ask for the same issues to be intended and/or tolerable?

@jairhenrique
Copy link
Collaborator Author

@hartwork The ruff dropped noqa because I didn't enable the E, F, W linters.
Enabling these linters I needed to turn noqa back.

@jairhenrique
Copy link
Collaborator Author

@hartwork I returned the numbers in noqa. Removed noqa tags are from "fixed" issues.

tests/integration/test_aiohttp.py Outdated Show resolved Hide resolved
tests/integration/test_boto3.py Outdated Show resolved Hide resolved
tests/integration/test_httpx.py Outdated Show resolved Hide resolved
vcr/__init__.py Outdated Show resolved Hide resolved
tests/integration/test_httpx.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
line-length = 110
target-version = "py38"

[tool.ruff.isort]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder what to do about the existing block tool.isort further up:

[tool.isort]
line_length = 110
known_first_party = "vcrpy"
multi_line_output = 3
use_parentheses = true
include_trailing_comma = true

In particular:

  • does ruff read any of that? (assuming: no)
  • should it be dropped? (assuming: yes)
  • can any of these options ported to ruff more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does ruff read any of that?

No 😄

should it be dropped?

Yes, done!

can any of these options ported to ruff more?

That's the default ruff configuration, but some settings configurations was not provided yet.

@hartwork hartwork changed the title Use ruff as linter Use ruff for linting instead of flake8 Jun 26, 2023
@hartwork hartwork changed the title Use ruff for linting instead of flake8 Use ruff for linting (instead of current flake8/isort/pyflakes) Jun 26, 2023
Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@jairhenrique thanks for the adjustments! I have checked locally that flake8 does not report any errors against the latest state (except the now expected "E501 line too long" because flake8 no longer knows we want 110 columns). The Python changes are great and if for some reason ruff should turn out to have serious drawbacks over flake8/isort/pyflakes latter, nothing is blocking a cheap return to these tools, so the risk is minimal. Good work! 👍

@jairhenrique jairhenrique merged commit 016a394 into master Jun 26, 2023
6 checks passed
@jairhenrique jairhenrique deleted the ruff branch June 28, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants