-
Notifications
You must be signed in to change notification settings - Fork 28
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
update pull request checklist #1336
update pull request checklist #1336
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1336 +/- ##
=======================================
Coverage 78.46% 78.46%
=======================================
Files 117 117
Lines 7866 7866
=======================================
Hits 6172 6172
Misses 1694 1694
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 think adding the build milestone is good.
Otherwise my impression is that most of this is too "in the weeds" for a checklist.
I'd add most of this to the "Tickets & PR" section of https://innerspace.stsci.edu/pages/viewpage.action?spaceKey=SCSB&title=Roman+Calibration+Pipeline
That page is more easily updated and flexible. Having the checklist be more generic also solves the problem of trying to keep both in sync.
That is good to have it on Innerspace; however, I think it is more important to have the checklist here as well, both to have the checklist be immediately visible (and less easily forgettable) and also have it accessible to outside contributors who can't see the Innerspace page. If I had a choice between keeping the checklist here or on Innerspace, I personally would choose here for that reason; however, I will yield to your suggestion if you think that is better |
If we want the instructions available to outside contributors I'd
suggest that it be added to the github wiki page.
…On 8/1/24 11:56 AM, Zach Burnett wrote:
I think adding the build milestone is good. Otherwise my
impression is that most of this is too "in the weeds" for a
checklist. I'd add most of this to the "Tickets & PR" section of
https://innerspace.stsci.edu/pages/viewpage.action?spaceKey=SCSB&title=Roman+Calibration+Pipeline
<https://innerspace.stsci.edu/pages/viewpage.action?spaceKey=SCSB&title=Roman+Calibration+Pipeline>
That page is more easily updated and flexible. Having the
checklist be more generic also solves the problem of trying to
keep both in sync.
That is good to have it on Innerspace; however, I think it is more
important to have the checklist here as well, both to have the
checklist be immediately visible (and less easily forgettable) and
also have it accessible to outside contributors who can't see the
Innerspace page. If I had a choice between keeping the checklist here
or on Innerspace, I personally would choose here for that reason;
however, I will yield to your suggestion if you think that is better
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/spacetelescope/romancal/pull/1336*issuecomment-2263414459__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!xEp5OJ8kr-KhhckAK4aGJTsz_LZt8ADUSiy6_eDoe6r5yfGrpAqeaYj36wUbCZ6oe4j3zd2UV1YW7Z-XNJs4uZ2j$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWJKQDYI2HSLR6U2AK3ZPJLCFAVCNFSM6AAAAABLWMP32SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGQYTINBVHE__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!xEp5OJ8kr-KhhckAK4aGJTsz_LZt8ADUSiy6_eDoe6r5yfGrpAqeaYj36wUbCZ6oe4j3zd2UV1YW7Z-XNOo8qjWT$>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
d40849a
to
6bbab79
Compare
move checklist instructions to comment update link to romancal
6bbab79
to
6ae2a84
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.
Approving since this has also gone in on the Webb side.
also added to the wiki page at https://github.com/spacetelescope/romancal/wiki/Pull-Request-Checklist |
analogous to spacetelescope/jwst#8670
old checklist
Checklist
CHANGES.rst
under the corresponding subsectionupdated relevant testsran regression tests, post a link to the Jenkins job below. How to run regression tests on a PRnew checklist
Tasks
24Q4_B15
(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_regtests
to update the truth filesnews fragment change types...
changes/<PR#>.general.rst
: infrastructure or miscellaneous changechanges/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.associations.rst
changes/<PR#>.scripts.rst
changes/<PR#>.mosaic_pipeline.rst
changes/<PR#>.patch_match.rst
steps
changes/<PR#>.dq_init.rst
changes/<PR#>.saturation.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.jump_detection.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.assign_wcs.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.photom.rst
changes/<PR#>.flux.rst
changes/<PR#>.source_detection.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.resample.rst
changes/<PR#>.source_catalog.rst