Repository navigation
DOC: Make ruff less rough for new contributors #16035
Description
We took the risk of increasing contribution barrier to have more automatic checks (started with flake8 replacement but looks like we keep adding stuff to https://github.com/astropy/astropy/blob/main/.ruff.toml ). Now the risk is catching up with us. As mentioned in https://astropy-report.orgmycology.com/sections/community_recommendations#beginner-resources (and also through the document), new contributors are intimidated by ruff checks and feels like perfection is killing practical contributions.
A newsletter or how-to on the new coding checks like ruff would be helpful.
It is mentioned briefly in https://docs.astropy.org/en/latest/development/codeguide.html#coding-style-conventions but that's about it.
Activity
MridulS commentedon Feb 14, 2024
Could reiterating the use of
pre-commithelp here? Withpre-commitproperly installed a new contributor shouldn't need to worry about these automatic checks, and the pre-commit runs ruff with fixes enabled. The ruff formatter in #16029 would also automatically fix the problems.Maybe putting
pre-commitprominently followed by agit commitis the answer here?taldcroft commentedon Feb 14, 2024
Another way to highlight ruff warnings immediately is encouraging and documenting use of an IDE that has a ruff plugin. In that way the contributor immediately sees any problems highlighted in their editor. We don't want to go into any detail here, just talk about the general principles and point to the ruff integrations docs that show what to do.
taldcroft commentedon Feb 14, 2024
This survey result should also serve as a cautionary tale about enforcing type checking, as opposed to encouraging it as a benefit for humans reading the code.
MridulS commentedon Feb 14, 2024
I have stayed away from the typing conversations in astropy as I am still pretty new to the community (and I have some strong-ish opinions about adding types to old scientific python code 😄 )
IMO adding types and then not enforcing them seems like an anti-pattern. Adding types makes the code very verbose and difficult for beginners to follow along (IMO). If we are paying the cost of adding types, we should at least make sure that these types are being tested and don't drift as people change code.
Adding types just for the sake of IDEs doesn't seem too great to me (again just opinions :) ).
If we do add types it makes sense to exploit all they offer, including strict type checking wherever there are types defined.
taldcroft commentedon Feb 14, 2024
@MridulS - I made a post in the astropy Slack #type-hinting channel. Feel free to express your opinions there, always valued!
eerovaher commentedon Feb 14, 2024
Would it be possible to set up a bot that would post
pre-commitinstallation instructions to a pull request if thepre-commit.cicheck fails?pllim commentedon Feb 14, 2024
Re: doc and bot -- ¿Por que no los dos?
If those passionate about these solutions have the capacity, does not hurt to have some draft PRs out to explore the different ideas. Thanks! 😄
nstarman commentedon Feb 14, 2024
I agree that the types should be useful! I think an easy place to start, and to make sure that the types stay up-to-date with the code is to turn on a runtime type checker in a pytest (cron?) job. The package beartype offers a pytest hook for this purpose. We can then also advertise that users can use beartype's standard import hook to turn Astropy into a runtime type checked library. I've been doing this in my own code recently and it's improved my coding happiness immeasurably. What I do is turn on runtime type checking for a test run, then turn it off for speed in production runs. This is only enabled by good typing in my code.
mhvk commentedon Feb 14, 2024
While I think the extra burdens we add in terms of style, etc., certainly don't help newcomers, I think it may also be the case that there simply is not so much to do for newcomers any more: looking through the 100 currently open PRs, nearly all are bugfixes, style fixes, or refactoring. This is not necessarily bad - it just means that astropy is getting fairly feature complete, like was the case for numpy for a long time already, and that the things that remain (like
uncertaintyandnddata) are quite hard. All this makes it a less interesting package for new contributors.But a bit more on topic, one suggestion would be to adjust the documentation a bit to really encourage people to start by raising an issue (something I found very hard to get even my own students to do for my own package!). In discussing an issue, one often finds out soon enough whether someone is interested in trying to contribute a fix/addition, and then there is already a bit of a relationship with someone who can review/help.
dhomeier commentedon Mar 7, 2024
Looking at issues labelled feature request however there are still nearly 400 open, but again very few of them are also labelled low effort or good first issue. That might reflect a bit of impostor syndrome in labelling, or the fact that we are short in maintainer time to guide and supervise newcomers on such projects. But then I have found that many of the refactoring PRs end up demanding a lot of maintainer time and input as well, because often it turns out to be not as simple as "let pre-commit implement all the Ruff rules".
eerovaher commentedon Mar 19, 2024
There's quite a lot information about using Ruff in #14818, but it would seem that issue is well hidden among the other 1200 open issues we have.
nstarman commentedon Mar 19, 2024
Pin?
pllim commentedon Mar 19, 2024
Why not document what is useful in #14818 in dev docs?
dhomeier commentedon Mar 19, 2024
Yes, this should be in the dev docs – the issue is or should be linked from all relevant PRs, so is not that hidden, but just having the recipes in the description they still seem to be easily overlooked.
eerovaher commentedon Mar 20, 2024
The instructions in #14818 describe addressing currently ignored Ruff rules and are less useful for normal development work. Furthermore, they were written for inexperienced contributors. My idea was that when the simpler Ruff rules are addressed then such detailed instructions will have outlived their usefulness, so there's no need to add them to the developer documentation.
The reason I mentioned #14818 here is that apparently there are new contributors who would like detailed instructions about using Ruff, but they seem to be completely unaware of #14818 despite it explicitly mentioning Ruff in its title and it having the
good first issueandPackage-novicelabels.pllim commentedon Mar 20, 2024
OK I pinned #14818 for now... Let's see if that helps in the meantime...