search

spacetelescope / jwst

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

Remove some unused modules #8630

Closed AlexKurek closed 4 months ago

AlexKurek commented 4 months ago

Resolves JP-nnnn

Closes #

This PR addresses ...

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

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 59.60%. Comparing base (536d5c6) to head (aae54e2).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8630 +/- ## ========================================== - Coverage 59.60% 59.60% -0.01% ========================================== Files 391 391 Lines 39286 39280 -6 ========================================== - Hits 23418 23412 -6 Misses 15868 15868 ```

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

emolter commented 4 months ago

started regression test run here

zacharyburnett commented 4 months ago

These changes look good to me, pending regtests; indeed, none of these modules are used. It seems to beg the question of why our ruff check didn't catch these earlier. @zacharyburnett maybe that's a question for you?

it looks like fits_generator and associations are excluded from linting: https://github.com/spacetelescope/jwst/blob/8af184cef5fe4553ae704cd1932d494beca07272/pyproject.toml#L276-L285

I copied this config from the previous flake8 config when moving it to pyproject.toml; I'm not sure why they were excluded in the first place

zacharyburnett commented 4 months ago

looks like these exclusions were added in 9be1e2793ead8eebd270345d6db97012cfd7f155

zacharyburnett commented 4 months ago

I ran rules F401 and F841 on jwst/fits_generator and jwst/associations and it found quite a few: https://github.com/spacetelescope/jwst/pull/8651

emolter commented 4 months ago

looks like these exclusions were added in 9be1e27

I think it was even earlier - that commit just split the exclusions across multiple lines.

Thanks for starting this discussion @AlexKurek - it sounds like we should go ahead and merge this one assuming the regression tests don't turn up any surprises, and then look at https://github.com/spacetelescope/jwst/pull/8651 separately. Does that sound ok to you @zacharyburnett ?

zacharyburnett commented 4 months ago

looks like these exclusions were added in 9be1e27

I think it was even earlier - that commit just split the exclusions across multiple lines.

Thanks for starting this discussion @AlexKurek - it sounds like we should go ahead and merge this one assuming the regression tests don't turn up any surprises, and then look at #8651 separately. Does that sound ok to you @zacharyburnett ?

sounds good to me!

zacharyburnett commented 4 months ago

These changes look good to me as well, and it does also sound like a good idea to remove those exclusions from the style checks. Thanks for spotting that, @emolter, and taking care of it @zacharyburnett.

made a separate PR to remove those exclusions https://github.com/spacetelescope/jwst/pull/8652