JP-3769: Style rules example case #9081
Conversation
There was a problem hiding this comment.
I have opinions!
- I'd like to include D400, D401, and D404 -- these are important components of the numpydoc style.
- I'd also like to include the N ruleset. We can make exceptions for files with important math variables that are clearer if they are capitalized.
- I think 130 characters is too long for the line-length. I'd prefer the standard 88.
- I don't think unit tests need docstings - we should blanket exclude tests/* from that requirement.
As configured, does this check for standard numpydocs section headers?
Sounds good, the current rules as you say are a bit more permissive than Numpy style. Let's see what others think.
Thanks for the input, let's see what other people think. I agree we could ignore these in certain places, but it's better to agree on the rules and be consistent than to have some rules that are only followed half the time and marked for ignoring later.
I prefer a longer line limit but let's see what the consensus is.
Test files are already excluded by the line |
Agreed, but I'd like to add one more comment here for consideration - shorter lines are much more readable for me when I need to make the font bigger for vision reasons.
Ah, I missed that! I just saw some reported issues with conftest files and assumed it was checking test_* files too. I'm not super concerned about code style in unit tests, but it might be nice to have the basic checks apply - it might help catch code errors in the tests themselves. |
|
a5bff5c has the first draft of this, with no N rules, relatively permissive D rules, and a max line length of 130 characters. 5925a28 adds Melanie's suggestions of changing the doc rules to comply with Numpy style as well as to add the naming (N) rules. I only added 8d4884a changes (in addition to the above) the line length to 88 for the formatter, i.e., d385721 adds numpydoc checks. Hopefully these give y'all some options to look at. Let me know your thoughts. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9081 +/- ##
==========================================
+ Coverage 77.74% 78.14% +0.40%
==========================================
Files 507 505 -2
Lines 46570 46243 -327
==========================================
- Hits 36207 36138 -69
+ Misses 10363 10105 -258
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
started regression tests here, just to see if there are any more failures caused by renaming stuff |
There was a problem hiding this comment.
I gave my thoughts on the formatting I liked and didn't like. I didn't note this every time I saw various things. Most of the formatting stuff is just my preference, which are mostly weak preferences, so if others prefer something different that's fine. I did, though, see many places with unnecessarily complicated and convoluted computations that could be simplified to be made easier to read, as well as more computationally efficient. This is something I do feel strongly about.
Thanks for these comments @kmacdonald-stsci, it's really helpful to have that feedback. I noticed that many of the style things you suggested are related to the auto-formatter, which I haven't played much with. So many of those can hopefully be accommodated. I will respond individually about those, if I just give it a "thumbs-up" emoji that means I read/understood/will take into account.
I completely agree; however, I don't think this PR is necessarily the place to change those. I'd like to keep the changes here restricted to those specifically required by the rules themselves, so that people can see what the rules themselves are going to do to the rest of the repository. Perhaps I can put in these changes after everyone has had a chance to comment about the rules. Otherwise, there is at least one AMI-related clean-up ticket where those changes may be appropriate. |
|
Codespell PR is here: #9097 With the line length and spell checker questions answered, this PR is as finished as I think I can make it |
| [tool.numpydoc_validation] | ||
| checks = [ | ||
| "all", | ||
| "EX01", # No examples section found | ||
| "SA01", # See Also section not found | ||
| "ES01", # No extended summary found | ||
| "GL08", # Object does not have a docstring. Ruff catches these, and allows more granular ignores. | ||
| "PR01", # Parameters not documented. Already caught by ruff. | ||
| "PR09", # Parameter description should finish with a period | ||
| "RT02", # First line of return should contain only the type | ||
| "RT05", # Return value description should finish with a period |
There was a problem hiding this comment.
Shoud this remain, or can we manage the numpydoc settings through the pre-commit config file?
There was a problem hiding this comment.
Yes, I believe they should remain. The numpydoc-validation call in pre-commit-config.yaml will still read pyproject.toml (and .ruff.toml), and plus having them here would allow them to still be picked up if calling numpydoc outside of the pre-commit.
There was a problem hiding this comment.
Having the pre-commit config set the numpydoc version and the pyproject set the validation checks makes my nose wrinkle a bit; similarly with the mypy configuration (and ruff configuration in the pre-commit config and .ruff.toml...)- if we're going to use pre-commit to manage our linters, can we set our configuration there? If the primary benefit of keeping these in the pyproject file is to have the settings picked up by non-pre-commit calls to the linters, didn't we already punt on that by dropping the versioning here?
It just seems a bit disorganized in how the configuration for various linters has been placed in the (three?) different configuration files.
| "PR01", # Parameters not documented. Already caught by ruff. | ||
| "PR09", # Parameter description should finish with a period | ||
| "RT02", # First line of return should contain only the type | ||
| "RT05", # Return value description should finish with a period | ||
| ] | ||
|
|
||
| [tool.mypy] |
There was a problem hiding this comment.
Are the mypy sections are useful if someone elects to install and run mypy separately from the listed requirements, or can these sections be removed?
There was a problem hiding this comment.
Yes, they are still useful as mypy will still pick up the config in pyproject.toml if you're running it from within the repository.
There was a problem hiding this comment.
Also, I'm pretty sure the mypy CI check also reads from here
|
@braingram @tapastro This is now ready for you to look again at the Also starting a final regtest run just in case https://github.com/spacetelescope/RegressionTests/actions/runs/13017040289 |
|
@tapastro After some engdb hiccup, the unit and regression tests are all passing. Should I take your thumbs-up emotes as approval of my responses to your two remaining questions? If you approve this I'll go ahead and merge it now. |
Resolves JP-3769
Closes #8841
Overview
This PR adds many more style rules to the repository, and applies them to the AMI module to show what the new rules would do to our code style.
The style is checked by two tools, ruff and numpydoc. Ruff was already part of our pre-commit CI, and this PR simply turns on many more style rules. Numpydoc, maintained by the numpy project, enforces numpy-like documentation style much more rigorously than ruff does. This PR adds numpydoc validation to the pre-commit job.
This PR adds numpydoc, ruff, and pre-commit to the optional testing dependencies, meaning that they will all be installed with
pip install jwst[test].Request for feedback
Don't be shy: NOW is the time to bring up any potential concerns, BEFORE we make these requirements global. It'll be a lot easier if we can avoid making global-scale edits after the fact. We need to come up with a final list of style rules. I've explained my logic for the first draft below, but I would love to hear your opinion:
How to use the new tools
pre-commit
If you install pre-commit locally, all of the checks that the pre-commit CI does will be run automatically when
git commitis executed, including the ruff and numpydoc checks added in this PR.pip install -U pre-committo installpre-commit install. This will put a pre-commit hook insidejwst/.git/hooks.git commit. This will run all the checks on any file that was modified in the commit, but not on other files. The commit will NOT occur unless all the checks pass.note: This was all possible prior to this PR, and nothing has changed about it except the details of the pre-commit job. But it 8000 may be more helpful now that there are multiple meaningful validations to run.
Ruff
pip install -U ruffto install.ruff.toml, search for[tool.ruff.lint.per-file-ignores], find the module you'd like to modify, and comment out the relevant ignore line. Ruff reads the config from.ruff.tomlso it won't work unless the files are un-ignored.ruff check .orruff check path/to/file.pyto report issuesruff check . --fixto fix issues Ruff deems "safe" fixes and report the restruff format .to apply auto-formatting. IMPORTANT: please ONLY apply this to modules you are editing! This does NOT respect the ignore rules and will apply formatting to the entire repository globally if you call it from the top-leveljwst/directory. I suggest applying this formatting in its own, clean commit, so if it gets messed up you can just revert it.numpydoc
pip install -U numpydocto install.pre-commit-config.yaml, scroll down to the bottom, find the module you'd like to modify, and remove the relevant line in the numpydoc-validation exclude list.numpydoc lint /path/to/file.pyto report issues. Unlike ruff, this does not support pointing to directories.Notes:
pyproject.toml, but its ignore list is inpre-commit-config.yamlHow to apply the rules to your favorite module
.pre-commit-config.yamlfile, scroll down to the bottom, find the module you'd like to modify, and remove the relevant line in the numpydoc-validation exclude list..ruff.tomlfile under the heading[lint.per-file-ignores], find the big list of every subfolder in the repository. COMMENT OUT the one you want to look at / fix.ruff format .from the module you are working on. This will auto-apply the formatting rules.ruff check .from the top-level JWST repository, or from the module you are working on. The output should show you style issues from whatever module(s) you commented out.Tips and tricks
ruff, so it would be good to be familiar with those if you go this route.ruff CODE000on Google:. Fix the issues that you can see easily, then rerun, and sometimes the harder-to-understand error messages resolve themselves.List of rules included and ignored
Numpydoc rules ignored (as of 1/23/2025)
Ruff rules included (as of 1/23/2025)
In coming up with the rule list, I turned various rulesets on and off globally for the repository and scanned the
ruff checkoutput, just to see if there were unexpected results or specific rules that we should definitely ignore. The draft list is included below (see also.ruff.toml) along with some comments on my decision-making. edit: this list is now up-to-date with what I feel is the consensus so far, but please feel free to chime in!input,id,match.import numpy as np. There was only one failure of this type in the whole repository.__init__.pyin all namespaces.type: ignoreandnoqa: are specific, not global.oswithPathlibin many cases. I kept this in, but possibly it's unnecessarily annoying?__slots__. There was only one failure of this type in the whole repository.print()statements in code.super()instead ofsuper(class, self). I feel there's no harm being explicit.open(file, "r")instead of justopen(file). I feel there's no harm being explicit.sys.versionandsys.version_info. There was only one failure of this type in the whole repository.Ruff rules rejected
@pytest.fixture(scope="function")with simply@pytest.fixture... yes, it's more compact, but the former is easier to understand without necessarily knowing the default by heart.Ruff rules I didn't consider
If you read the ruff docs yourself, you'll notice some rulesets are not on this list. That's because I read the description and assumed they weren't particularly useful or relevant. But I could be convinced otherwise.
Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst