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

chore: make sure local ruff runs don't touch vendored #618

Merged
merged 4 commits into from
Aug 4, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented May 10, 2024

It's useful to run ruff locally sometimes, like to add --unsafe-fixes. This ensures it doesn't format or change something it shouldn't. See #617.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.03%. Comparing base (78b9ea9) to head (74c0d54).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #618   +/-   ##
=======================================
  Coverage   71.03%   71.03%           
=======================================
  Files          13       13           
  Lines        1084     1084           
=======================================
  Hits          770      770           
  Misses        314      314           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DimitriPapadopoulos
Copy link
Contributor

I suggest you remove this:

wheel/pyproject.toml

Lines 71 to 74 in 1e00742

[tool.black]
extend-exclude = '''
^/src/wheel/vendored/
'''

It had been forgotten when shifting from black to ruff in 83b77e5.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Alex Grönholm <alex.gronholm@nextday.fi>
@agronholm
Copy link
Contributor

We already have this in place which makes this PR redundant.

@agronholm agronholm closed this Aug 4, 2024
@DimitriPapadopoulos
Copy link
Contributor

Nope. it's not redundant. It's set for pre-commit, but not for standalone ruff runs.

@agronholm
Copy link
Contributor

Why not just do pre-commit run -a?

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Aug 4, 2024

Because I simply run ruff.

@agronholm
Copy link
Contributor

Well, you could just as easily run pre-commit run -a ruff, no need to even install ruff explicitly?

@DimitriPapadopoulos
Copy link
Contributor

Yet, I do have ruff installed. When I run ruff, results are not consistent with pre-commit.

@agronholm
Copy link
Contributor

How so?

@agronholm
Copy link
Contributor

Are you sure you're running the same version in both cases?

@DimitriPapadopoulos
Copy link
Contributor

Of course. It's just that pre-commit skips vendored, while standalone ruff does not.

@henryiii
Copy link
Contributor Author

henryiii commented Aug 4, 2024

It’s hard to add flags when running pre-commit, like --unsafe-fixes, so it’s handy to run locally sometimes. Unlike flake8, there’s no variations based on what else is installed, so it’s safe to run it outside pre-commit’s venvs.

Though if all you want to do is run it only with no special flags, I would recommend pre-commit run -a ruff.

@agronholm
Copy link
Contributor

Of course. It's just that pre-commit skips vendored, while standalone ruff does not.

Oh, that's the only thing you meant by results not being consistent?

@agronholm
Copy link
Contributor

It’s hard to add flags when running pre-commit, like --unsafe-fixes, so it’s handy to run locally sometimes. Unlike flake8, there’s no variations based on what else is installed, so it’s safe to run it outside pre-commit’s venvs.

Though if all you want to do is run it only with no special flags, I would recommend pre-commit run -a ruff.

Ah...that's a valid argument. Yeah, I don't see a way to pass flags through pre-commit run. When I needed to use --unsafe-fixes, I just temporarily added that flag to .pre-commit-config.yaml.

@DimitriPapadopoulos
Copy link
Contributor

I tend to avoid pre-commit, it doesn't work well outside containers and virtualenvs on older Linux distributions, typically the oldest still maintained Ubuntu LTS.

@agronholm agronholm reopened this Aug 4, 2024
@agronholm agronholm merged commit 46c2389 into pypa:main Aug 4, 2024
18 checks passed
@henryiii henryiii deleted the henryiii/chore/ruffex branch August 4, 2024 14:11
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