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

infra: fix ruff configuration and add a few checks #573

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 31, 2023

I was looking at adding a few checks (trying to check the validity of replacing isort), and discovered fixes/checks weren't being applied. It turns out build is being ignored by default, so the whole package in src/build is being ignored! For the pre-commit check, I don't think the default excludes really make much sense, and I've got other projects with a submodule named build, too, so while it especially affects us, it does affect others too (scikit-build-core is one with a build submodule). I see why it might be ignored by default if you aren't running in pre-commit.

CC @charliermarsh, would it make sense to pass --exclude="" or something like that for the pre-commit hook? Maybe the respect gitignore flag? Pre-commit has it's own system for skipping files, it seems broken that it's not being respected. Well, at least if one of the excluded directories is something you actually want included. I wouldn't mind keeping the other directories there so this can be run outside of pre-commit more easily, but I can't have ".../build" excluded. Maybe there just needs to be an "include" or "dont-exclude"?

@charliermarsh
Copy link

@henryiii - I've gotta think on this -- the confluence of all the different settings is making my head hurt 😅 I don't think we can blindly set --exclude "" because users could have valid, custom exclusions set there, that could include Python files that are checked in to Git, and so we'd be linting them inadvertently.

@layday
Copy link
Member

layday commented Jan 31, 2023

Did you mean to replace isort with ruff? You haven't opted into it.

@henryiii
Copy link
Contributor Author

Not yet. I hit this and wanted to fix it first. And it might not be configurable enough yet.

@layday
Copy link
Member

layday commented Jan 31, 2023

But you removed isort's config.

@henryiii
Copy link
Contributor Author

Oops, thought I reverted that. 😅

henryiii added a commit to scikit-build/scikit-build-core that referenced this pull request Feb 1, 2023
Some of our files are being skipped because they have "build" in the
name. Noticed this in pypa/build#573, and turns
out problems were being masked here too.

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

henryiii commented Feb 1, 2023

That's better. Since we have custom spacing set for isort (not sure why?), I'm pretty sure we can't switch isort to Ruff (without changing it, that is).

I'm not sure what the correct solution to the exclude problem is, but it does seem that the current situation is not ideal. But I might just be more susceptible to it, due to working on a lot of building stuff. :) I do think it's better to err on the side of not silently ignoring directors, as a linter can't tell if something is missed, it just never reports anything for that directory. I wouldn't mind adding manually adding extend-excludes for directories that are pretty common but not hidden.

@layday
Copy link
Member

layday commented Feb 1, 2023

I think you accidentally reverted the ruff config changes too 😅

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii changed the title chore: fix ruff configuration and add a few checks infra: fix ruff configuration and add a few checks Feb 1, 2023
@henryiii henryiii merged commit 7a76acc into pypa:main Feb 1, 2023
@henryiii henryiii deleted the henryiii/chore/fixruff branch February 1, 2023 17:33
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