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
DM-22677: Relocate scripts to python module #216
Conversation
e20e290
to
838db10
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.
Two minor comments on one script that probably apply to all of them.
print(config, file=outfh) | ||
|
||
|
||
def main(): |
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.
Would it be worthwhile having this take an args
to pass to parse_args()
that would take sys.argv[1:]
or similar? That seems like it's a missing piece of making this more usable, but I also don't really a scenario where we'd actually use that.
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 believe I'm following standard practice here: https://developer.lsst.io/stack/argparse-script-topic-type.html?highlight=autoprogram
Note | ||
---- | ||
Notes | ||
----- |
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 I've also got this fix on a branch for you to review 🙂 .
Exceptions lead to bad exit status anyhow so there is no need to catch the exception just to return a bad exit status.
No description provided.