DM-30373: Add butler collection-chain command line #533
Conversation
timj
force-pushed
the
tickets/DM-30373
branch
5 times, most recently
from
b9c93db
to
801088d
Compare
There was a problem hiding this comment.
Looks good. If you're up for a little more work, a --remove X option that just does
children = list(registry.getCollectionChain(parent))
children.remove(X)
registry.setCollectionChain(parent, children)
and a --pop that does
children = registry.getCollectionChain(parent)[1:]
registry.setCollectionChain(parent, children)
would be super useful, too.
|
@TallJimbo I've added this but you should take a look before I merge. Did you mean Rather than add a |
| @repo_argument(required=True) | ||
| @click.argument("parent", required=True, nargs=1) | ||
| @click.argument("children", required=False, nargs=-1) | ||
| @click.option("--pop", is_flag=True, default=False, |
There was a problem hiding this comment.
@TallJimbo Could also do this as --mode=pop and let CHILDREN be given as integer indexes into the chain (so as to match list.pop) -- defaulting to pop off the end.
There was a problem hiding this comment.
(see other thread - I think the difference here is that I don't think popping off the end is a good default)
There was a problem hiding this comment.
In Perl you shift to move things off the front of the list and pop to remove things from the end. I do worry that calling this pop goes against the default python definition of pop. Maybe something like:
$ butler collection-chain REPO --mode=pop PARENT index index index
would work where we default index to -1? (and since this is using CHILDREN allow multiple indices).
There was a problem hiding this comment.
How about --mode=pop --side=front, or --mode=popfront?
There was a problem hiding this comment.
I've just implemented the --mode=pop defaulting to element 0 and allowing --mode=pop -1 to remove from the end.
There was a problem hiding this comment.
The main problem actually is that -1 is an option so you have to do -- -1 to make it work. From what you say that's not going to be a common problem and the positive integer always works as desired.
| help="Update mode: " | ||
| "'redefine': Create new chain or redefine existing chain with the supplied CHILDREN. " | ||
| "'remove': Modify existing chain to remove the supplied CHILDREN. " | ||
| "'extend': Modify existing chain to extend it with the supplied CHILDREN.") |
There was a problem hiding this comment.
Would --mode=prepend be useful?
There was a problem hiding this comment.
Yes, and more useful than extend, I think. We may represent these things as Sequences in Python, but conceptually we (pipetask in particular) really treat them as LIFO stacks, where all of the mutations happen at the front (yeah, maybe I should have implemented them as reversed-lists in Python, but I figured ordering the container according to the search order is more important).
So I really did mean for --pop to act on the front, and I really only care that we provide ways to act on the front in general, but I could easily believe that also providing ways to act on the end is the best way to make it clear what all of the actions actually do.
Everything crashes in `butler --help` if one of the local commands fails to load but it's better to report everything whilst complaining rather than being impenetrable.
Click will call _get_command multiple times for `butler --help` and if something is going wrong with the imports this will result in a cascade of import warnings. Work around that by caching the imports the first time around and only warning once.
This allows chained collections to be modified.
Pop is now a mode that allows integer indices to specify the elements to pop. Add extend option. Also now prints the current contents of the chain on completion.
Checklist
doc/changes