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-16828: Add Job viewer to lsst.verify #31
Conversation
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.
This looks handy, thanks for writing it.
My primary concern is preventing bitrot on this: is there a way to write a simple test of it with a sample job output? You could either lift the "working code" out into lsst.verify.something
or maybe just run this as an executable from a test via subprocess.run()
.
bin.src/inspect_job.py
Outdated
print("Syntax: %s <job file> [[job file]...]" % sys.argv[0]) | ||
sys.exit(1) | ||
for filename in sys.argv[1:]: | ||
if len(sys.argv) > 2: |
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.
Maybe better off putting this logic in main()
and using ArgumentParser
to manage the argv
stuff.
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.
ArgumentParser
seems kind of overkill here, don't you think? It's not a proper CLI, just a list of files.
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.
Either way. I find using argparse to even handle "just a list of files" to be more robust, but its your call.
About putting the core business code into the Python package itself, see https://github.com/lsst/verify/blob/master/python/lsst/verify/report.py for something similar. Report has slightly different information content (it so far focusses on tabulating pass/fail status). But it shows how you might create a class that could either render plain text on the command line, or something like HTML in a notebook. |
Ok, I'll try to move everything into |
7b77f62
to
48ebc5a
Compare
On second thought, I think creating a display object may be best deferred to a separate ticket. Making a display class that's specific to So I think it would be better to just add a unit test, and solve the problem of multiple output formats later. |
That seems like a good approach. |
64693a9
to
096dc4f
Compare
The output is unit tested for basic properties, but not an exact match to any particular formatting.
096dc4f
to
6093eba
Compare
This PR adds the
inspect_job.py
script for getting a quick and uncluttered summary of a persisted job. User documentation has been added to the Sphinx build.