spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
561 stars 167 forks source link

lint `jwst/fits_generator` and `jwst/associations` #8652

Closed zacharyburnett closed 2 months ago

zacharyburnett commented 2 months ago

jwst/fits_generator and jwst/associations are currently excluded from linting with ruff and therefore pre-commit

After merging this, when devs change files in those modules, pre-commit should lint their changes

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 37.50000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 59.58%. Comparing base (ccf871a) to head (67878cc). Report is 9 commits behind head on master.

Files Patch % Lines
jwst/associations/lib/update_path.py 0.00% 4 Missing :warning:
jwst/fits_generator/input_file_types.py 0.00% 4 Missing :warning:
jwst/associations/lib/rules_level2_base.py 25.00% 3 Missing :warning:
jwst/fits_generator/main.py 0.00% 1 Missing :warning:
jwst/fits_generator/objects.py 0.00% 1 Missing :warning:
jwst/fits_generator/template.py 50.00% 1 Missing :warning:
jwst/fits_generator/verifiers.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8652 +/- ## ========================================== + Coverage 59.56% 59.58% +0.01% ========================================== Files 391 391 Lines 39285 39274 -11 ========================================== - Hits 23402 23401 -1 + Misses 15883 15873 -10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zacharyburnett commented 2 months ago

@melanieclarke I applied the fixes suggested by Ruff as best I could, but I have a few questions about lambdas and unused variables (see the review comments)

melanieclarke commented 2 months ago

It looks like there are some unit test failures. We'll also need to run regression tests for these changes, when the unit tests are passing.

I'll check out the lambdas and unused variables.

melanieclarke commented 2 months ago

there is another test that's failing that looks import related (jwst/associations/tests/test_registry.py::test_registry_match). I'm not sure which one is causing that failure.

I think this one is because the imports in rules_level2b and rules_level3 are missing ASN_SCHEMA.

zacharyburnett commented 2 months ago

there is another test that's failing that looks import related (jwst/associations/tests/test_registry.py::test_registry_match). I'm not sure which one is causing that failure.

I think this one is because the imports in rules_level2b and rules_level3 are missing ASN_SCHEMA.

yep that fixed it!

❯ pytest -n auto jwst/associations/tests/test_registry.py
============================== test session starts ==============================
platform darwin -- Python 3.11.9, pytest-8.2.2, pluggy-1.5.0
crds_context: jwst_1253.pmap
rootdir: /Users/zburnett/projects/jwst
configfile: pyproject.toml
plugins: cov-5.0.0, asdf-3.3.0, ci_watson-0.7.0, doctestplus-1.2.1, jwst-1.15.2.dev14+gf6562101f, xdist-3.6.1, requests-mock-1.12.1
8 workers [6 items]     
......                                                                    [100%]
=============================== 6 passed in 3.44s ===============================
zacharyburnett commented 2 months ago

Changes look good to me, but I don't see the regression test run linked anywhere even though the box is checked - did I miss it somewhere? I approve this conditional on no regtest failures.

whoops, I forgot about that, here's the regtest run now: https://github.com/spacetelescope/RegressionTests/actions/runs/9975534597

emolter commented 2 months ago

regtest failures look the same as the ones on main, i.e., related to the architecture differences when switching over to the new GitHub actions workflow. So they are unrelated to this PR