Skip to content
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

Command heirarchy/grouping #71

Closed
jaswilli opened this issue Oct 17, 2016 · 6 comments
Closed

Command heirarchy/grouping #71

jaswilli opened this issue Oct 17, 2016 · 6 comments
Labels
question A question or open discussion usability Pertains to CLI ergonomics & comfort
Milestone

Comments

@jaswilli
Copy link
Contributor

Do we want to promote certain commands to be top-level commands? Example: "endpoint search" is currently globus transfer endpoint search. Should it be globus search?

Other commands where this would make sense?

@jaswilli jaswilli added this to the 1.0 milestone Oct 17, 2016
@jaswilli jaswilli added question A question or open discussion usability Pertains to CLI ergonomics & comfort labels Oct 17, 2016
@jaswilli jaswilli mentioned this issue Oct 17, 2016
36 tasks
@jaswilli
Copy link
Contributor Author

Candidates:

transfer endpoint search -> endpoint-search
transfer endpoint ls -> endpoint-ls

@sirosen
Copy link
Member

sirosen commented Oct 17, 2016

Just to have this on-record: I don't think it's a good idea to promote any other commands, at least for now.
The clean siloing of functionality by subservice becomes more beneficial the more services we add, and the chances for naming conflicts or confusion within a given subservice is highly unlikely.

We consider these two operations (endpoint search and ls) safe to promote because they operate in terms of one of our fundamental model objects, namely Endpoints. As such, the chances of a conflict with another service are exceedingly low.

However, getting back on my high-horse now, I think that requiring calls to a service to be done as explicit commands against that service is a very clean and natural design in the multi-service world. It also maps very well back onto the types of client objects with which a person is likely to interact if they need to move onto the SDK.

@sirosen
Copy link
Member

sirosen commented Oct 17, 2016

It turns out that this is a lot messier than I thought it would be, for the simple reason that re-attaching commands (my original plan) does not have the right semantics.
A click.Command object has its name as an attribute.
If we re-attach globus transfer ls as a globus subcommand, it will be globus ls. We cannot rename it at that point.

There is a tutorial on aliasing MultiCommand subcommands: http://click.pocoo.org/6/advanced/
That means that re-attaching globus transfer ls to be globus endpoint-ls requires fairly deep modification of the globus command.
Given that we're already wrapping the top level command in a custom class to implement the custom excepthooks, that should be simple -- except that it results in a cyclic dependency between globus_cli.services.transfer and globus_cli.parsing. Resolving that sounds highly unattractive, especially since the "solution" in that case is an ugly hack as far as I'm concerned and will probably play havoc on the helptext.

The most realistic solution for this we can do is a pair of new commands which forward all of their options to the endpoint search and ls commands. That means setting a couple of options to forward arguments (ignore_unknown_options and allow_extra_args), and using Context.forward to pass all arguments through.
Unfortunately, the Click docs make me nervous about doing this, saying

Generally this pattern is strongly discouraged because it’s not possibly [sic] to losslessly forward all arguments

It also seems like those options might be set on the sub-context used by the new command? But it's not quite clear how context settings modified at command instantiation time get passed down, so that's not certain.
I'm also unsure about the --help output we would get in that case. Because the wrapper command doesn't do any argument processing, it should pass --help through, which might result in weird output which doesn't respect the invoking command name.

Overall, this is looking like a huge slog for something relatively minor.

sirosen added a commit to sirosen/globus-cli that referenced this issue Oct 17, 2016
The aliases are constructed in terms of a rename and an existing command
object, and assign nearly all of the same attributes to the alias
command. The only exception is the name, which is obviously altered in
the process.
Importantly, this is a type of click.Command, and is not generic enough
to safely apply to a Group or MultiCommand -- the attributes for those
types are different, and the passthrough of original command attributes
to the new command's constructor will not cover the differences.

The AliasCommand importantly inherits the original commands params, so
it does not need to blindly forward options as unprocessed values. That
saves us from some of the concerns over doing that.

Make
 - `globus endpoint-search` -> `globus transfer endpoint search`
 - `globus endpoint-ls` -> `globus transfer ls`
as the first two such aliases.

Resolves globus#71
@sirosen
Copy link
Member

sirosen commented Oct 17, 2016

On a hunch, I looked at the way that the parameters themselves are stored on a command object -- as I hoped, they're just an attribute of the command.
I have a PR I'm opening now that leverages this to make aliases a new type of click.Command built off of a given target command. In my initial testing, it seems to work well.

@sirosen sirosen added the blocked There's some reason we can't do this label Oct 18, 2016
@sirosen
Copy link
Member

sirosen commented Oct 18, 2016

Per notes on the AliasCommand PR, marking this as blocked until we see a clear and correct path forward. Hopefully one that doesn't involve biting off completely custom helptext.

@corpulentcoffee
Copy link
Contributor

@sirosen sirosen removed the blocked There's some reason we can't do this label Oct 28, 2016
sirosen added a commit to sirosen/globus-cli that referenced this issue Oct 29, 2016
Everything in globus_cli.services.transfer moves to one of three
locations:
Most move to globus_cli.commands
Everything in the transfer.helpers moves to a new module at
`globus_cli.services.transfer`, some other functionality moves into that
module.
Line-by-line shlex processing moves to globus_cli.parsing

On that last note, fixes globus#103 by inspecting exit status on caught
SystemExits.
Because this does the dramatic refactor, it's also fair to say this
resolves globus#71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A question or open discussion usability Pertains to CLI ergonomics & comfort
Projects
None yet
Development

No branches or pull requests

3 participants