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.

Sign up for GitHub

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

Jump to bottom

fix return code for asn_from_list and skycell_asn, remove unused scripts, cleanup skycell_asn #1538

Merged
merged 7 commits into from
Dec 10, 2024
Merged

fix return code for asn_from_list and skycell_asn, remove unused scripts, cleanup skycell_asn #1538

merged 7 commits into from
Dec 10, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Nov 25, 2024 •
edited
Loading

This PR fixes the return code for asn_from_list and skycell_asn when they succeed (see #1535).

Which sorting out that fix I saw that:

  • "schema_editor" and "schemadoc" are registered scripts that do not exist. This PR removes their registration from pyproject.toml
  • The python function (not method) "skycell_asn" takes 1 argument "self". This PR updates the function signature to follow conventions of a non-method function. Further improvements are likely called for if this is expected to be part of the public API (it is mentioned in the documentation).

I left a number of in-line comments with some important ones about unused command line arguments for "skycell_asn" (which are not modified in this PR).

Closes #1535

Regtests all pass: https://github.com/spacetelescope/RegressionTests/actions/runs/12018368084

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (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#>.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

@github-actions github-actions bot added dependencies Pull requests that update a dependency file associations labels Nov 25, 2024
@github-actions github-actions bot added documentation Improvements or additions to documentation testing labels Nov 25, 2024
@braingram braingram force-pushed the asn_from_list_return branch from 9648a5a to c1afe2a Compare November 25, 2024 20:06
braingram
braingram commented Nov 25, 2024
View reviewed changes
romancal/associations/asn_from_list.py
@@ -43,7 +43,7 @@ def asn_from_list(items, rule=DMS_ELPP_Base, **kwargs):
return asn


class Main:
def _cli(args=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Including a class as a script in pyproject.toml as is done on main:

romancal/pyproject.toml

Line 95 in bee71ba

asn_from_list = "romancal.associations.asn_from_list:Main"

results in an installed script containing the following line:

sys.exit(Main())

This is problematic because Main() is "truthy" resulting in the script returning 1 on success. By switching this to a function (with no return value) the script becomes:

sys.exit(main())

and the no return value (None) is "falsey" leading to a return code of 0 (success) when the script succeeds.

braingram
braingram commented Nov 25, 2024
View reviewed changes
romancal/associations/skycell_asn.py
help="The release product when creating the association",
)

parser.add_argument(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is unused both on main and with this PR.

braingram
braingram commented Nov 25, 2024
View reviewed changes
romancal/associations/skycell_asn.py
"The rule to base the association structure on." ' Default: "%(default)s"'
),
)
parser.add_argument(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is unused both on main and with this PR.

braingram
braingram commented Nov 25, 2024
View reviewed changes
romancal/associations/skycell_asn.py
help="Root string for file to write association to",
)

parser.add_argument(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is unused both on main and with this PR.

@braingram braingram changed the title fix return code for asn_from_list and skycell_asn, remove unused scripts fix return code for asn_from_list and skycell_asn, remove unused scripts, cleanup skycell_asn Nov 25, 2024
@braingram braingram marked this pull request as ready for review November 25, 2024 20:42
@braingram braingram requested a review from a team as a code owner November 25, 2024 20:42
@braingram

This comment was marked as outdated.

Sign in to view
@codecov Codecov
Copy link

codecov bot commented Nov 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.04878% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.11%. Comparing base (5c515b8) to head (800fd01).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
romancal/associations/skycell_asn.py 65.00% 7 Missing ⚠️
romancal/associations/asn_from_list.py 90.47% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1538   +/-   ##
=======================================
  Coverage   78.10%   78.11%           
=======================================
  Files         117      117           
  Lines        7642     7639    -3     
=======================================
- Hits         5969     5967    -2     
+ Misses       1673     1672    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mairanteodoro
mairanteodoro reviewed Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

Looks good to me!

romancal/associations/asn_from_list.py Outdated Show resolved Hide resolved
romancal/associations/skycell_asn.py Show resolved Hide resolved
@braingram braingram force-pushed the asn_from_list_return branch from c4dc053 to 7e026eb Compare December 3, 2024 22:21
mairanteodoro
mairanteodoro reviewed Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@braingram braingram force-pushed the asn_from_list_return branch from cdd9038 to 800fd01 Compare December 9, 2024 17:42
mairanteodoro
mairanteodoro approved these changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

Still looks good to me!

@braingram braingram merged commit 6cb4b51 into spacetelescope:main Dec 10, 2024
29 of 30 checks passed
@braingram braingram deleted the asn_from_list_return branch December 10, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mairanteodoro mairanteodoro mairanteodoro approved these changes

@ddavis-stsci ddavis-stsci Awaiting requested review from ddavis-stsci

Assignees
No one assigned
Labels
associations dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation regression_testing testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

asn_from_list has non 0 exit code on apparent success
2 participants
@braingram @mairanteodoro