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 ruff to pre-commit hooks #41

Closed
aazuspan opened this issue Jun 29, 2023 · 9 comments · Fixed by #43
Closed

Add ruff to pre-commit hooks #41

aazuspan opened this issue Jun 29, 2023 · 9 comments · Fixed by #43
Assignees
Labels
dx Developer experience, tools, efficiency, etc.
Milestone

Comments

@aazuspan
Copy link
Contributor

aazuspan commented Jun 29, 2023

pyupgrade standardizes syntax, including type annotations, to a minimum Python version. That should make it easier to maintain consistency, especially as we drop support for older versions of Python.

Good overview of the tool: How to Upgrade Syntax with pyupgrade

EDIT: Rather than adding pyupgrade, we could switch to ruff which supports almost all of the features of pyupgrade, flake8, isort, and a variety of other linters (including flake8 extensions like bugbear and docstrings.) In addition to being much faster than our current tools, this would consolidate almost everything into a single tool.

@grovduck let me know if you have any preferences here on tooling. Otherwise I'll probably try migrating sankee to ruff over the weekend to get a better idea of whether there are any sticking points, and report back.

@aazuspan aazuspan added the dx Developer experience, tools, efficiency, etc. label Jun 29, 2023
@aazuspan aazuspan added this to the 0.1.0 milestone Jun 29, 2023
@aazuspan aazuspan self-assigned this Jun 29, 2023
@aazuspan aazuspan changed the title Add pyupgrade to pre-commit hooks Add pyupgrade to pre-commit hooks (or use ruff) Jun 30, 2023
@aazuspan
Copy link
Contributor Author

aazuspan commented Jul 3, 2023

Quick update: I moved sankee over to ruff, and as you can see it allowed me to simplify the pre-commit config substantially. I replaced everything but black with ruff, and added some additional checks in the process (you can see all the supported options here).

The transition was painless, so unless you object @grovduck I think I would recommend we do the same for sknnr. Probably the only downside would be if you like to run tools individually or use more advanced options that might not be available in the ruff CLI.

@grovduck
Copy link
Member

grovduck commented Jul 3, 2023

@aazuspan, man, good timing on this one ... I was just complaining over in #40 that I'm having issues with some of the pre-commit (specifically sourcery). I'm guessing sourcery is not in ruff and I think that would be just fine.

I'm all for this change if you think it will work well. Do you get a sense for whether I should perhaps merge in #40 (with the duplicated tests) before you tackle this or vice versa? I'm happy to sit tight if you think this would be a pretty straightforward transition and also happy to merge in the PR if that makes sense. I know we could also do it concurrently and I'm fine with that as well.

Quick question without doing a lick of research: will ruff make it easier to avoid the typing errors that I've managed to push over and over, i.e. does it have any tox-like capabilities to check the matrix of python versions?

@aazuspan
Copy link
Contributor Author

aazuspan commented Jul 3, 2023

will ruff make it easier to avoid the typing errors that I've managed to push over and over, i.e. does it have any tox-like capabilities to check the matrix of python versions?

I was hoping there was a lint rule in there that would catch these kind of backwards compatibility issues (ruff is aware of the target Python version), but I don't think there is.

Maybe we need to reconsider including tox. I guess the other alternative would be to always develop in the minimum supported version environment, but that's a hassle...

@aazuspan aazuspan changed the title Add pyupgrade to pre-commit hooks (or use ruff) Add ruff to pre-commit hooks Jul 3, 2023
aazuspan added a commit that referenced this issue Jul 5, 2023
- Remove sourcery from pre-commit per discussion in #40
- Replace flake8 and isort with ruff
- Add new checks via ruff including pyupgrade

This also brings everything into compliance with the new checks:

- Replace nested context managers with single block
- Remove unused param from `test_estimators_support_dataframes`
- Move noqas in docstrings. Ruff wants them placed at the end of
the multiline string, not on the offending line.
- Remove unused noqas.
@aazuspan aazuspan linked a pull request Jul 5, 2023 that will close this issue
@aazuspan
Copy link
Contributor Author

aazuspan commented Jul 6, 2023

@grovduck, regarding testing across Python versions, I just come across a config option in Hatch that allows you to specify a matrix of Python versions and test against one or all of them. Hatch still needs to be able to find interpreters for the requested versions locally (not sure of the best way to get that set up reliably), but I'm thinking this might be a simpler solution than adding in tox.

@grovduck
Copy link
Member

grovduck commented Jul 7, 2023

I just come across a config option in Hatch that allows you to specify a matrix of Python versions and test against one or all of them

@aazuspan, this sounds great!

Hatch still needs to be able to find interpreters for the requested versions locally (not sure of the best way to get that set up reliably)

I poked around for a bit, but I couldn't find any guidance on how to do this in hatch's docs or elsewhere. Have you found any links to know the mechanism for how hatch might find the different interpreters?

This sounds like a great solution if, as you say, we can set up a reliable way to set up the interpreter finding!

@aazuspan
Copy link
Contributor Author

aazuspan commented Jul 7, 2023

I haven't found any official docs, but this discussion links to this code which shows that hatch uses the virtualenv.discovery.builtin.get_interpreter function to locate Python versions. The docs for virtualenv have some details on how Python versions are discovered, which seems to be based on having them defined on the system path.

That same discussion and this one both suggest pyenv as a solution for setting up multiple Python versions, but installation on Windows is a hassle. I've been experimenting with that, but still haven't gotten anything to work quite right...

Python environment management isn't any fun 😫

@grovduck
Copy link
Member

grovduck commented Jul 7, 2023

Wow, I can tell you are already waist-deep into it! I know you'll want to solve this - if there's anything I can do to help test, please let me know.

@aazuspan
Copy link
Contributor Author

Adding ruff is resolved by #43, but I'll leave this open for the moment until we resolve or split off the question of Python environment testing with Hatch or tox.

@aazuspan
Copy link
Contributor Author

In the interest of cleaning out old issues, I'm going to close this now and hold off on opening a new one until we find ourselves needing local Python version testing (fingers crossed we won't!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx Developer experience, tools, efficiency, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants