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-28929: Allow non-string HTCondor attributes #19
Conversation
Update doc/conf.py to new version Add a :no-inherited-members: option to the automodapi directive in ctrl_bps/doc/lsst.ctrl.bps/index.rst to workaround bug. Since having to test building the docs, included minor text changes: Update pipeline syntax from ':' to '#' to match stack changes. Fix one missed capitalization change (qgraph_file to qgraphFile).
tests/test_lssthtc.py
Outdated
htcondor = None | ||
|
||
|
||
@unittest.skipIf(not htcondor, "Warning: Missing HTCondor API. Skipping") |
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 is fine, although I think I'd prefer skipIf(htcondor is None, ...)
to be explicit. You can also say:
@unittest.skipUnless(htcondor is not None, "...")
value : `str` | ||
String that needs to have characters escaped. | ||
value : `Any` | ||
Value that needs to have characters escaped if string |
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.
Don't we need a period at the end of this sentence?
else: | ||
pval = value | ||
|
||
print(f'+{key} = {pval}', file=stream) |
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.
Could you please add a brief comment here hinting why this check in necessary here? Since it is so "close" to identical check in htc_escape()
it took me longer that I'd like to admit to figure out that they serve two different purposes.
The HTCondor plugin's code assumed that attribute value was a string. But Walltime on NCSA's DAC cluster needs to be an integer. So this ticket fixed the plugin code. Also some unrelated doc changes.