globus / globus-cli

A command line interface to Globus
https://docs.globus.org/cli
Apache License 2.0
74 stars 22 forks source link

ls -R support #7

Closed jimPruyne closed 8 years ago

jimPruyne commented 8 years ago

An option on ls mimicking ls -R behavior would be helpful. Maximum depth options and global policy are probably prudent. In JSON output mode, behavior such that the path (at least relative to the root path given on the command line) is displayed would be needed.

sirosen commented 8 years ago

@jimPruyne and I discussed this a little bit further.

My plan is to finish my current refactoring before starting on this, but making it the next feature I try to tackle. I'll release the refactored version as 0.2.0, so this will be targeted as part of 0.2.1

Like aws s3 cp, I'll start with --recursive, but no short-form option. In general, staying away from short options will lead to more readable scripts, so I think this is a good rule of thumb.

There's a question about what to name the recursion depth as an argument. Should it be --recursive-depth-limit N with a reasonable default? We could do --recursive-depth-limit -1 for unbounded, which I actually think is pretty clear. @jimPruyne Curious as to what you think would be a nice name for this.

$ globus transfer ls \
  --endpoint-id abc123 --path '/some/path/' \
  --recursive --recursive-depth-limit 10

is verbose, but very clear. Unless there's a better name out there, that's what I'll go with.

I'm thinking the behavior for -F TEXT mode should be pretty much the same as ls -R, with empty lines before dir names, dir names given including the starting path, and trailing colons after dir names. However, like non-recursive output, I don't plan to do column formatting.

Behavior for -F JSON is much simpler, actually. Just produce a DATA array of file items, taken verbatim out of the API responses. Whether or not to include some of the higher-level metadata is a question of taste, but including the root path is very easy, so why not. I'm not sure that the other data is of much use, and I'd rather start with the minimal set of things to support, so I'll be sticking to the path and nothing else.

A note of caution to myself here: I'll be trying to behave like the API and avoid trailing slashes on ls results, for consistency.

jimPruyne commented 8 years ago

I'm fine with --recursive-depth-limit though as noted it is somewhat verbose. Assume that providing this without the corresponding --recursive is considered erroneous input (simply --recursive N with an optional N seems nice, but ambiguous if the last flag assuming there are default arguments that are not preceded with a --flag (should --path behave that way (i.e. we simply provide the path(s) as the final argument(s) to the ls command?)).

I don't like the idea of depth -1 for infinite. Too easy to get hung up on loops in the filesystem "tree".

All output paths/names (either TEXT or JSON) should complete the complete path in the tree (at least relative to --path)

sirosen commented 8 years ago

I don't like the idea of depth -1 for infinite. Too easy to get hung up on loops in the filesystem "tree".

I'll leave a depth of -1 off the menu for now. You can always give us a depth of 1000 or something similarly absurd. I was considering the possibility of having unlimited depth as an "at your own risk" kind of option, but after some thought I agree that it's not really worth it.

I'm fine with --recursive-depth-limit though as noted it is somewhat verbose. Assume that providing this without the corresponding --recursive is considered erroneous input (simply --recursive N with an optional N seems nice, but ambiguous if the last flag assuming there are default arguments that are not preceded with a --flag (should --path behave that way (i.e. we simply provide the path(s) as the final argument(s) to the ls command?)).

Although I would like --recursive N for this particular use-case, the feature-reject in click for this exact kind of behavior has some salient points about how this hurts the extensibility and learnability of the CLI: https://github.com/pallets/click/issues/370

Basically, it's the exact point you raised of positional arguments becoming ambiguous.

Potentially confusing behavior is leading me to avoid positional arguments as much as possible. For example, how do you read globus transfer async-transfer <uuid1> <uuid2> <uuid3> <uuid4>? Which UUID is the source endpoint ID, and which one is the destination path? Passing --path for ls may seem silly today, but future features may render that clarity really useful.

All output paths/names (either TEXT or JSON) should complete the complete path in the tree (at least relative to --path)

I'm a little confused by this. For JSON output, I assume it means you want to be able to join the --path you supplied with the filenames produced, and not traverse some kind of nested structure.

However, for TEXT, do you mean you want output similar to find <dirname>, rather than ls -R <dirname>? The find output is actually a lot easier to work with, IMO, and even simpler for me to produce, but I'm not sure that it's the right behavior -- I don't know if ls should behave as much like the coreutils ls as it reasonably can, or if it should just try to be reasonable/useful.

jimPruyne commented 8 years ago

I'll leave a depth of -1 off the menu for now. You can always give us a depth of 1000 or something similarly absurd.

Agree though I wouldn't be too surprised if someone thinks a max of something on the order of 10s would be preferred. Not sure of the actual utility of such an enforced max since an enterprising (read DoS wannabe) could make multiple calls anyway. Ultimately, a more useful max might be in terms of total number of files returned (paging?) or number of api calls made. That also opens a can of worms, so this is a semantically good way to go I think.

Potentially confusing behavior is leading me to avoid positional arguments as much as possible. For example, how do you read globus transfer async-transfer ? Which UUID is the source endpoint ID, and which one is the destination path? Passing --path for ls may seem silly today, but future features may render that clarity really useful.

Agreed. Though the transfer case may also have a model that people are comfortable with by way of scp type behavior.

All output paths/names (either TEXT or JSON) should complete the complete path in the tree (at least relative to --path)

I'm a little confused by this. For JSON output, I assume it means you want to be able to join the --path you supplied with the filenames produced, and not traverse some kind of nested structure.

Yes. That's what I'm getting at.

However, for TEXT, do you mean you want output similar to find , rather than ls -R ?

In spite of what I originally typed, I was thinking primarily about the JSON case. For TEXT, I think following ls -R (as in ls when not outputting to a tty) is the way to go, though I could see some benefit of find style also. But, given that this is an ls command, make it look like-ish the usual ls.

The find output is actually a lot easier to work with, IMO, and even simpler for me to produce, but I'm not sure that it's the right behavior -- I don't know if ls should behave as much like the coreutils ls as it reasonably can, or if it should just try to be reasonable/useful.

Least surprise to me is if I type something with ls in the name, I get something that looks kinda like I expect ls to look. find output when my fingers have typed ls will likely confuse me (perhaps only for a fraction of a second, but still...)

sirosen commented 8 years ago

Cool. I feel like I have the necessary detail. Starting on this now.

@jimPruyne Are you cool with reviewing the result? I can give you instructions on how to install the CLI directly from the repo, if you're willing to play with it a bit and see if it "handles" the right way.

jimPruyne commented 8 years ago

yep, will do on review of behavior if not code.

sirosen commented 8 years ago

This will go to pypi with 0.2.1