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 Codespell to pre-commit #9097

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Jan 27, 2025

Relates to JP-3769

This PR adds the Codespell spell-checker to the pre-commit workflow, and fixes all spelling issues in the repository. This is part of a larger effort to add more code and documentation style rules to the repository; see #9081

Thanks to @braingram for the suggestion to include Codespell.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.datamodels.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.fits_generator.rst
  • changes/<PR#>.set_telescope_pointing.rst
  • changes/<PR#>.pipeline.rst

stage 1

  • changes/<PR#>.group_scale.rst
  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.emicorr.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.ipc.rst
  • changes/<PR#>.firstframe.rst
  • changes/<PR#>.lastframe.rst
  • changes/<PR#>.reset.rst
  • changes/<PR#>.superbias.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.rscd.rst
  • changes/<PR#>.persistence.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.charge_migration.rst
  • changes/<PR#>.jump.rst
  • changes/<PR#>.clean_flicker_noise.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.gain_scale.rst

stage 2

  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.badpix_selfcal.rst
  • changes/<PR#>.msaflagopen.rst
  • changes/<PR#>.nsclean.rst
  • changes/<PR#>.imprint.rst
  • changes/<PR#>.background.rst
  • changes/<PR#>.extract_2d.rst
  • changes/<PR#>.master_background.rst
  • changes/<PR#>.wavecorr.rst
  • changes/<PR#>.srctype.rst
  • changes/<PR#>.straylight.rst
  • changes/<PR#>.wfss_contam.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.fringe.rst
  • changes/<PR#>.pathloss.rst
  • changes/<PR#>.barshadow.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.pixel_replace.rst
  • changes/<PR#>.resample_spec.rst
  • changes/<PR#>.residual_fringe.rst
  • changes/<PR#>.cube_build.rst
  • changes/<PR#>.extract_1d.rst
  • changes/<PR#>.resample.rst

stage 3

  • changes/<PR#>.assign_mtwcs.rst
  • changes/<PR#>.mrs_imatch.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.exp_to_source.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.tso_photometry.rst
  • changes/<PR#>.stack_refs.rst
  • changes/<PR#>.align_refs.rst
  • changes/<PR#>.klip.rst
  • changes/<PR#>.spectral_leak.rst
  • changes/<PR#>.source_catalog.rst
  • changes/<PR#>.combine_1d.rst
  • changes/<PR#>.ami.rst

other

  • changes/<PR#>.wfs_combine.rst
  • changes/<PR#>.white_light.rst
  • changes/<PR#>.cube_skymatch.rst
  • changes/<PR#>.engdb_tools.rst
  • changes/<PR#>.guider_cds.rst

@tapastro
Copy link
Contributor

I think automatic fixes are fine with me - it follows our expected ruff behavior, and "bad" autocorrects should be caught in code review - at least as much as non-auto-corrects should be caught by the dev and the review...

I blame the typo on rushing to get my comment in before the conversation passes me by 😄

@emolter
Copy link
Collaborator Author

emolter commented Jan 28, 2025

Sounds good. Just to keep the two PRs cleanly separated, with this one relating to Codespell only, I put the dev tag in the other one: 3389375

Does this look good to you @braingram @tapastro ? I just tested it on a new conda env and it seems to work. If so, I will update CONTRIBUTING.md about best practices for setting up our development environment, then get that PR, and finally update the config for this one and try to get it merged soon too.

And that sounds fine RE turning on auto-fixes

@braingram
Copy link
Collaborator

braingram commented Jan 28, 2025

Sounds good. Just to keep the two PRs cleanly separated, with this one relating to Codespell only, I put the dev tag in the other one: 3389375

Does this look good to you @braingram @tapastro ? I just tested it on a new conda env and it seems to work. If so, I will update CONTRIBUTING.md about best practices for setting up our development environment, then get that PR, and finally update the config for this one and try to get it merged soon too.

And that sounds fine RE turning on auto-fixes

My vote is to have pre-commit manage the ruff, codespell, etc versions and installs. That's how things work in the CI and maintaining separate versions may lead to complications. Let's say someone right now installs the dev dependencies. They will get ruff 0.9.3 whereas pre-commit lists 0.5.2. These will report and fix different rule sets.

@emolter
Copy link
Collaborator Author

emolter commented Jan 28, 2025

My vote is to have pre-commit manage the ruff, codespell, etc versions and installs. That's how things work in the CI and maintaining separate versions may lead to complications. Let's say someone right now installs the dev dependencies. They will get ruff 0.9.3 whereas pre-commit lists 0.5.2. These will report and fix different rule sets.

Melanie made the point that not everyone uses (or likes to use) pre-commit and may prefer to run tools individually. I agree with your point that keeping the versions in-sync is somewhat of a problem though

@tapastro
Copy link
Contributor

Sounds good. Just to keep the two PRs cleanly separated, with this one relating to Codespell only, I put the dev tag in the other one: 3389375
Does this look good to you @braingram @tapastro ? I just tested it on a new conda env and it seems to work. If so, I will update CONTRIBUTING.md about best practices for setting up our development environment, then get that PR, and finally update the config for this one and try to get it merged soon too.
And that sounds fine RE turning on auto-fixes

My vote is to have pre-commit manage the ruff, codespell, etc versions and installs. That's how things work in the CI and maintaining separate versions may lead to complications. Let's say someone right now installs the dev dependencies. They will get ruff 0.9.3 whereas pre-commit lists 0.5.2. These will report and fix different rule sets.

Using pre-commit is fine, but I don't think part of our workflow should include requiring devs run pip install pre-commit - shouldn't our dependency files be able to handle that? Can we make a dev tag that installs pre-commit? I'll try to understand how pre-commit sets version requirements in the CI, but would we use a .pre-commit-config.yaml here?

@emolter
Copy link
Collaborator Author

emolter commented Jan 28, 2025

I'll try to understand how pre-commit sets version requirements in the CI, but would we use a .pre-commit-config.yaml here?

We already do use .pre-commit-config.yaml and yes, that is where pre-commit sets version requirements.

One thing I don't really like about suggesting to only use pip install pre-commit && pre-commit install is that after those two commands, running (e.g.) ruff check . gives -bash: ruff: command not found. It feels like whatever commands we suggest that devs use should have the end result that all of the tools will be installed and work individually as well as together.

It would be really nice if there was an easy/robust way to keep the versions in sync... but I think this goes counter to the philosophy of pre-commit based on this stackoverflow answer from the creator of pre-commit. And reading more there may be good reasons pre-commit chose to set itself up this way. Not that this improves the situation.

@emolter
Copy link
Collaborator Author

emolter commented Jan 29, 2025

Regression tests started here: https://github.com/spacetelescope/RegressionTests/actions/runs/13036860875

Build failures are the same as on all PRs right now

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM. There's a merge conflict which looks minor. All regtests passed and I think given the scope of changes a maitainer review will also be required.

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

This PR makes my inner copy-editor so happy. One question/objection, but mostly this looks great.

jwst/ami/utils.py Outdated Show resolved Hide resolved
@melanieclarke
Copy link
Collaborator

I'm happy with this, but I'll hold off on approval in case Tyler has comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants