-
Notifications
You must be signed in to change notification settings - Fork 26
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
Align style checks with jwst #383
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
===========================================
+ Coverage 67.59% 78.17% +10.57%
===========================================
Files 115 115
Lines 5932 5145 -787
===========================================
+ Hits 4010 4022 +12
+ Misses 1922 1123 -799 ☔ View full report in Codecov by Sentry. |
fe3d7b5
to
1ce97af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for putting this together, Brett!
The changes to the CI and pre-commit look good to me. I had some suggestions for following up on this PR, but none of them would have to be handled now.
Most of my comments are related to places where I think it wouldn't be overly disruptive to fix the errors instead of noqa: ignore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this pending passing downstream JWST unit tests. Probably wouldn't hurt to re-run the regression tests too out of an abundance of caution, since you accepted at least one of the "unsafe" fixes I suggested.
Rerunning regtests here: https://github.com/spacetelescope/RegressionTests/actions/runs/13039378253 |
Regtests show only unrelated errors. |
Co-authored-by: Ned Molter <[email protected]>
a27e516
to
0c91732
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good to me now, or at least, we have separate issues tracking additional changes.
Why is the Python 3.10 test failing now? I don't see how it was modified by the changes here.
The 3.10 test failure: |
spacetelescope/jwst#9081 introduced major changes to the code style checks in jwst.
Since this package primarily serves jwst this PR updates the style checks here to more closely match those in jwst.
Regtests: https://github.com/spacetelescope/RegressionTests/actions/runs/13039378253show 1 error
test_exec_time_0_crs
which I'm chalking up to a slower-than-usual runner which took 11 instead of 10 seconds. I expect these may need to be re-run after review so I'll open this PR for review now.EDIT: see updated regtests comment: #383 (comment)
Branch protections will need to be updated after this PR is merged to check for pre-commit success.
Tasks
docs/
pageno-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)jwst
regression tests with this branch installed ("git+https://github.com/<fork>/stdatamodels@<branch>"
)news fragment change types...
changes/<PR#>.feature.rst
: new featurechanges/<PR#>.bugfix.rst
: fixes an issuechanges/<PR#>.doc.rst
: documentation changechanges/<PR#>.removal.rst
: deprecation or removal of public APIchanges/<PR#>.misc.rst
: infrastructure or miscellaneous change