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
Print default values for strun --help #101
Print default values for strun --help #101
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #101 +/- ##
==========================================
+ Coverage 54.16% 54.71% +0.55%
==========================================
Files 25 25
Lines 3024 3010 -14
==========================================
+ Hits 1638 1647 +9
+ Misses 1386 1363 -23
☔ View full report in Codecov by Sentry. |
16381ac
to
fb7ce2e
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.
These changes look good to me, but I'll hold off approving this until I @zacharyburnett gets back about my question.
] | ||
|
||
setup(scripts=scripts) | ||
setup() |
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.
@zacharyburnett is the setup.py
even needed anymore? This seems to have been added back by #76.
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.
It does not seem to be needed (as per the setuptools
and pip
docs) now that we are using PEP621 and PEP518. There used to be guidance to include a "shim" setup.py
but that seems to have been removed.
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.
Whatever the oldest version of python we're supporting (3.9), the current conda distribution of the latest 3.9.x patch (and the one from python.org) should include a version of setuptools and pip that comply, i.e. don't require a setup.py or setup.cfg. If that's the case, delete away.
@@ -4,7 +4,7 @@ | |||
|
|||
import pytest | |||
|
|||
from ..step import AbstractDataModel | |||
from stpipe.datamodel import AbstractDataModel |
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 have been meaning to move this test module outside of src
. Thanks
5ade4f8
to
5e2b73a
Compare
The CI failure for the jwst tests is expected, due to a change in DQ flag names merged into stdatamodels/master, but the CI tests use the latest release version. |
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.
Looks OK to me, and the system/installation aficionados seem to agree, so I hereby approve. As long as it doesn't break my pipeline! ;-)
@zacharyburnett @WilliamJamieson Did you want any further changes to this before merging? |
Let me run the romancal and jwst regression tests before I merge this. |
for more information, see https://pre-commit.ci
5e2b73a
to
a0b9463
Compare
4af4cbd
into
spacetelescope:master
This PR allows for printing of the a step parameter's default value if it exists when invoking
It also moves
strun
to entry points and puts a test into the location that tests are supposed to be, not inline with the code insrc/stpipe
.Before:
This PR:
Resolves #72
Resolves #99