DM-25385 make a pipetask command based on click #266
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timj
approved these changes
Jun 22, 2020
| @@ -38,7 +38,7 @@ def test_repoBasic(self): | |||
| """Test the most basic required arguments.""" | |||
| self.run_test(["register-instrument", "here", "a.b.c"], | |||
| self.makeExpected(repo="here", | |||
| instrument=("a.b.c",))) | |||
| instrument=["a.b.c"])) | |||
There was a problem hiding this comment.
Was this change required or you just preferred the list?
There was a problem hiding this comment.
this is because the split_commas util function was added as a callback to the instrument_option. split_commas returns a list. I wonder if it should return a tuple tho? It's a question of mutability vs immutability right?
There was a problem hiding this comment.
split_commas returning an immutable object is probably more correct given the usage scenario. You never expect someone to add more entries after they get it back. Sometimes in the implementation code that would require a copy so we tend not to do it.
| if self.parameterType == ParameterType.ARGUMENT: | ||
| f.__doc__ = addArgumentHelp(f.__doc__, self.help) | ||
| return click.argument("instrument", | ||
| required=self.required, |
There was a problem hiding this comment.
Were these reordered to be alphabetical?
n8pease
force-pushed
the
tickets/DM-25385
branch
2 times, most recently
from
637c6ac
to
2055979
Compare
n8pease
force-pushed
the
tickets/DM-25385
branch
3 times, most recently
from
620659a
to
59ecedb
Compare
tests the lsst.Log portion, which is loaded by butler only if another package as setup lsst.Log.
This test also runs in butler, but butler does not depend on lsst.log so in CI those parts of CliLog are not tested with daf_butler. Since obs_base does depend on lsst.log, running the test here is more complete.
n8pease
force-pushed
the
tickets/DM-25385
branch
from
59ecedb
to
f2a66bc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.