Repository navigation
Unleash numpydoc pre-commit - #20284
Conversation
github-actions
Bot
added
testing
skip-changelog-checks
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
We use this at Rubin a lot and it's very helpful. My only caveat is that it only helps if there is a prek (faster pre-commit) github action to check that the code is compliant for people who don't use pre-commit. I didn't see such an action but maybe it's got a different name. |
I don't think it is a problem for us. Our developer guide clearly states that we use pre-commit and we expect them to. |
|
what's the downside of adding the github action though? That will let people know who forgot to read that and are doing their first PR and will be a no-op for people who are following the guidelines. The pre-commit action is trivial to add: https://github.com/lsst/images/blob/main/.github/workflows/pre-commit.yaml will work directly as is for you. |
|
Re: prek -- Please open a new issue to discuss this, as I think that topic is a little out of scope here. Personally, I don't see the point. I don't know who |
Oh. So you already have a pre-commit github action. I misunderstood. |
| '\.__str__$', | ||
| '\.__repr__$', | ||
| ] | ||
| exclude_files = [ # don't process filepaths that match these regex |
There was a problem hiding this comment.
So does this leave any files that are actually checked, or is it just a proof of concept for the pre-commit job at this point (or will pre-commit still check new files)?
There was a problem hiding this comment.
Yes, currently I ignored everything to get the pre-commit job to pass. I envision where I could possibly remove one of the ignores and fix it as POC for this PR. But the rest can be a series of small(ish) follow-up PRs. Eventually we would only ignore the stuff we don't care about (tests, deprecated modules, etc).
technically: no. We run pre-commit.ci, which is a separate service than Github Actions. re: |
|
I wasn't trying to confuse things by advocating prek on the client side. I was saying that I was using prek on the GitHub action side but since you do have a separate pre-commit CI run then that was irrelevant. |
|
@pllim how did you choose the config (ie what to exclude) - was this copied from another package? |
I was locked in a mansion with them, then every night I presented one of them with a rose until these are left. jk jk I started with That is why the first TODO item is dev discussions, so we can fine-tune the lists. 😺 |
|
Sounds good. I’m on my phone so can't do it right now but we should also compare to the reproject config. |
| """ | ||
|
|
||
| [tool.numpydoc_validation] | ||
| checks = [ |
There was a problem hiding this comment.
@astrofrog here is the reproject settings:
[tool.numpydoc_validation]
checks = [
"all", # report on all checks, except the below
"EX01",
"SA01",
"SS06",
"ES01",
"GL08",
]At the time of writing, reproject list is a subset of this proposed PR.
| '^astropy/constants/.*', | ||
| '^astropy/convolution/.*', | ||
| '^astropy/coordinates/.*', | ||
| '^astropy/cosmology/.*', |
There was a problem hiding this comment.
👁️
Color me interested in addressing this.
[ci skip]
[ci skip]
[ci skip]
[ci skip]
[ci skip]
76b795a to
f5b30da
Compare
|
@pllim - rebased just to see if CI is happier |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
I personally think we should just do that and we can always tweak the config the first time we address one of the subpackages
|
So @astrofrog , do you want me to check off any of those TODO items here? Or you just want to merge this as-is? At least the devdocs? |
|
Tom R said no need for devdocs. So merging! |
|
Follow-up issue is #20353 |
Description
This pull request is to close #15386
TODO