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

Move more configs to pyproject.toml #770

Merged
merged 10 commits into from
Jun 27, 2024
Merged

Conversation

alexfikl
Copy link
Contributor

It's the future! 🎉

This mostly removes pytest.ini and moves most of the things from setup.cfg here.

Flake8 doesn't support pyproject, so it's not moved, but there is a project that hacks support in there if we want:
https://github.com/john-hen/Flake8-pyproject

.test-conda-env-py3.yml Outdated Show resolved Hide resolved
@alexfikl alexfikl marked this pull request as draft June 19, 2024 13:49
@alexfikl
Copy link
Contributor Author

alexfikl commented Jun 19, 2024

The failures seem to be related to the release of numpy 2.0, but I'm not sure what it doesn't like since pyopencl should be built from source on the CI. Right?

EDIT: For reference, this fails on main too: https://github.com/inducer/pyopencl/actions/runs/9630426577/job/26560978801

@matthiasdiener
Copy link
Contributor

Flake8 doesn't support pyproject, so it's not moved, but there is a project that hacks support in there if we want: https://github.com/john-hen/Flake8-pyproject

FWIW, I've been using flake8-pyproject in some of my projects, and it seems to work fine (see e.g. https://github.com/matthiasdiener/orderedsets/blob/551786a5b254372c097b3f676808d8e49dc0dcc8/pyproject.toml#L33)

@alexfikl alexfikl force-pushed the more-pyproject branch 2 times, most recently from 94e0c09 to 0c93679 Compare June 25, 2024 08:16
@alexfikl
Copy link
Contributor Author

FWIW, I've been using flake8-pyproject in some of my projects, and it seems to work fine (see e.g. matthiasdiener/orderedsets@551786a/pyproject.toml#L33)

Yeah, I've used it too and it worked without any issues.

One small problem with flake8 at the moment is also that the scripts in ci-support look for some strings in setup.cfg (e.g. # enable-isort), so that requires some more work..

@alexfikl alexfikl marked this pull request as ready for review June 25, 2024 08:22
@inducer
Copy link
Owner

inducer commented Jun 25, 2024

inducer/ci-support@3cbd99a should allow seamless use of flake8-pyproject.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 97 to 91
select = [
"B", # flake8-bugbear
"C", # flake8-comprehensions
"D", # pydocstyle
"E", # pycodestyle
"F", # pyflakes
"I", # flake8-isort
"N", # pep8-naming
"Q", # flake8-quotes
"W", # pycodestyle
]
Copy link
Contributor Author

@alexfikl alexfikl Jun 26, 2024

Choose a reason for hiding this comment

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

I added this too, but I'm not very convinced, more for a small discussion. (I don't recall if you mentioned this at some point?)

I thought about it mainly for two reasons:

  • removing the enable-isort (and others) grepping in ci-support: with this it can just install all the plugins it wants and the project will select the ones it wants.
  • if we move to ruff, all these are bundled anyway and we'll need to select the ones we want.
  • it's unlikely at this point, but this would also guard against new added error codes.

Copy link
Owner

Choose a reason for hiding this comment

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

I think I prefer this, even. Getting ci-support to understand this won't be too much of a lift. I like it mostly because it allows us to avoid making up our own idiotic config bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it mostly because it allows us to avoid making up our own idiotic config bits.

Yeah, that too! We can leave it in then, and slowly teach ci-support and everybody else to slowly migrate.

Copy link
Owner

@inducer inducer Jun 26, 2024

Choose a reason for hiding this comment

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

Now that I think of it: Why do we even need this? If we have the tools listed in project.optional-dependencies.test, we could just use that.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. I guess the "ruff" argument stands. :/ How close is ruff a this point? Should we just switch to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yeah, that's a good point. Sounds easier to standardize on some optional dependencies group name and just work with that if it exists!

I kind of like that better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I guess the "ruff" argument stands. :/ How close is ruff a this point? Should we just switch to it?

It doesn't have a lot of the fancier pylint checks that actually execute the code (or whatever they do), but everything else is there and seems to be working very nicely!

I would say it's pretty solid at this point (since they got bored and re-implemented black as well :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a list of the pylint checks that are implemented
astral-sh/ruff#970

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, let's say ruff is the future then, replacing flake8+plugins. Pylint can live for the moment.

@alexfikl
Copy link
Contributor Author

alexfikl commented Jun 26, 2024

With inducer/ci-support#32 we could also remove setup.cfg completely. The only one that remains is pylint with the special .pylintrc-local.yml 😖

@alexfikl alexfikl force-pushed the more-pyproject branch 4 times, most recently from 687f78b to 2c2cf28 Compare June 27, 2024 07:47
@alexfikl
Copy link
Contributor Author

@inducer This is looking good to me and all the pieces from ci-support seem to be working nicely. Is there anything else missing? I would leave the transition to ruff for another PR.

@inducer
Copy link
Owner

inducer commented Jun 27, 2024

So I tried moving the whole thing to ruff, and I liked what I saw. 🤷 It's a bit of churn, but it's just really quite nice. I rather like the LSP integration. I imagine we'll do that (with time) across the whole stack.

@inducer
Copy link
Owner

inducer commented Jun 27, 2024

LMK how you feel about this.

@inducer
Copy link
Owner

inducer commented Jun 27, 2024

It's certainly much less of a mess than flake8.

@inducer inducer force-pushed the more-pyproject branch 4 times, most recently from 055e7fa to 10db0a2 Compare June 27, 2024 16:03
@alexfikl
Copy link
Contributor Author

alexfikl commented Jun 27, 2024

So I tried moving the whole thing to ruff, and I liked what I saw. 🤷 It's a bit of churn, but it's just really quite nice. I rather like the LSP integration. I imagine we'll do that (with time) across the whole stack.

I'm all for it! It looks very nice and it seems to have caught some extra stuff 😁

It's a bit of churn

Have you tried ruff check --fix? I mostly replaced isort with ruff check --fix --select=I and it works very well, not sure about the other rules..

@inducer inducer merged commit 0ceaee4 into inducer:main Jun 27, 2024
17 checks passed
@alexfikl alexfikl deleted the more-pyproject branch June 27, 2024 18:36
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.

None yet

3 participants