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

braingram · 2024-11-25T19:38:09Z

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 running at: https://github.com/spacetelescope/RegressionTests/actions/runs/12018368084

Tasks

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

braingram · 2024-11-25T19:39:08Z

pyproject.toml

-schema_editor = "romancal.scripts.schema_editor:main"
-schemadoc = "romancal.scripts.schemadoc:main"


Neither of these exist and attempting to run schema_editor or schemadoc after installing romancal produces an error.

braingram · 2024-11-25T20:11:15Z

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):


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 · 2024-11-25T20:14:25Z

romancal/associations/skycell_asn.py

@@ -20,11 +20,11 @@
 logger.setLevel("INFO")


-def skycell_asn(self):
-    """Create the associaton from the list"""
+def skycell_asn(filelist, output_file_root, product_type, release_product):


I broke apart the def skycell_asn(self) function definition here. A docstring improvement would also be worth doing as this is a documented function. I did not make major changes in this PR because:

the splitting of self is already an improvement

I'm not sure what are the desired descriptions of these variables

A code suggestion for the docstring is appreciated.

braingram · 2024-11-25T20:14:57Z

romancal/associations/skycell_asn.py

+        help="The release product when creating the association",
+    )
+
+    parser.add_argument(


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

braingram · 2024-11-25T20:15:08Z

romancal/associations/skycell_asn.py

+            "The rule to base the association structure on." ' Default: "%(default)s"'
+        ),
+    )
+    parser.add_argument(


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

braingram · 2024-11-25T20:15:43Z

romancal/associations/skycell_asn.py

+        help="Root string for file to write association to",
+    )
+
+    parser.add_argument(


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

braingram · 2024-11-25T20:43:35Z

oldestdeps failure is addressed in #1539

codecov · 2024-11-25T21:05:36Z

Codecov Report

Attention: Patch coverage is 76.19048% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.67%. Comparing base (bee71ba) to head (c4dc053).

Files with missing lines	Patch %	Lines
romancal/associations/skycell_asn.py	61.90%	8 Missing ⚠️
romancal/associations/asn_from_list.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1538      +/-   ##
==========================================
- Coverage   76.68%   76.67%   -0.01%     
==========================================
  Files         120      120              
  Lines        7831     7829       -2     
==========================================
- Hits         6005     6003       -2     
  Misses       1826     1826

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

fix return code for asn_from_list and skycell_asn, remove unused scripts

f0489a9

github-actions bot added dependencies Pull requests that update a dependency file associations labels Nov 25, 2024

braingram commented Nov 25, 2024

View reviewed changes

rename main to _cli

abfd8ea

github-actions bot added documentation Improvements or additions to documentation testing labels Nov 25, 2024

drop Main from asn_from_list and skycell_asn

c1afe2a

braingram force-pushed the asn_from_list_return branch from 9648a5a to c1afe2a Compare November 25, 2024 20:06

braingram commented Nov 25, 2024

View reviewed changes

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

add changelog

fccfba6

braingram marked this pull request as ready for review November 25, 2024 20:42

braingram requested a review from a team as a code owner November 25, 2024 20:42

braingram requested a review from ddavis-stsci November 25, 2024 20:42

remove Main use in regtest

c4dc053

github-actions bot added the regression_testing label Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

braingram commented Nov 25, 2024 •

edited

Loading

braingram Nov 25, 2024

braingram Nov 25, 2024

braingram Nov 25, 2024

braingram Nov 25, 2024

braingram Nov 25, 2024

braingram Nov 25, 2024

braingram commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

		schema_editor = "romancal.scripts.schema_editor:main"
		schemadoc = "romancal.scripts.schemadoc:main"

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

Are you sure you want to change the base?

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

Conversation

braingram commented Nov 25, 2024 • edited Loading

Tasks

steps

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram Nov 25, 2024

Choose a reason for hiding this comment

braingram commented Nov 25, 2024 • edited Loading

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

braingram commented Nov 25, 2024 •

edited

Loading

braingram commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading