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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

code command completions for Bash & Zsh #56670

Merged
merged 7 commits into from Jan 4, 2019

Conversation

Projects
None yet
5 participants
@segevfiner
Copy link
Contributor

segevfiner commented Aug 17, 2018

Add Bash & Zsh completions that will be packaged for all operating systems under resources/completions. For the deb/rpm packages, they will also be automatically installed to the
correct location (Hopefully... 馃槣).

  • "resources/completions/zsh/_code" was copied and modified from
    zsh/Completion/X/Command/_code, hopefully that's OK licensing wise.
  • Should we try to complete the arguments to folder-uri using _urls? Those are not standard
    URLs most of the time, so I think not.
  • code, like most commands, allows long option arguments to be after an equals or in the next
    argument. In Zsh completion's _arguments function you need to add an equals after the
    argument to support this. Otherwise it won't complete after an equals. But this also makes Zsh
    complete the option with an equals. It seems inconsistent currently, with somea arguments
    using an equals and some not, but maybe there was some rationale behind it. I'm not sure what
    other Zsh completion scripts generally do about this. What should be done? Iv'e kept it the
    same in Bash, except there it always completes both after an equals and in a a separate
    argument so it's not stricly required to add an equals sign to long options there it's just a
    matter of style if we prefer to complete with an equals or not.
  • Better completion for -g/--goto in Bash?
  • I reverted the help strings from the Zsh completion script to be closer to the VS Code --help message, see discussions below.

P.S. I think the appdata.xml copied in gulp for the rpm is not getting installed anywhere by the
spec file, I think the directory for it also changes with distro versions.

Part of #56497

@segevfiner segevfiner changed the title Shell completions Shell completions for Bash & Zsh Aug 17, 2018

@segevfiner segevfiner changed the title Shell completions for Bash & Zsh code command completions for Bash & Zsh Aug 17, 2018

@sandy081 sandy081 requested a review from joaomoreno Aug 20, 2018

@joaomoreno joaomoreno added this to the Backlog milestone Aug 21, 2018

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Sep 13, 2018

Cool stuff. I see some unchecked checkboxes. Should I wait for you to settle on what to do there, or should we merge a first draft of this and fix things in the future?

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 13, 2018

The unsolved tasks/checkboxes are things I wasn't sure about. It can be merged as is as long as there is no licensing issue and everything else looks OK. But if you have an idea on how to handle any of those than I can fix them accordingly before that.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Sep 13, 2018

"resources/completions/zsh/_code" was copied and modified from
zsh/Completion/X/Command/_code, hopefully that's OK licensing wise.

You're gonna have to reach out to those guys, actually. Let me know their answer and licensing terms.

Should we try to complete the arguments to folder-uri using _urls? Those are not standard URLs most of the time, so I think not.

Nah, it's fine.

code, like most commands, allows long option arguments to be after an equals or in the next
argument. In Zsh completion's _arguments function you need to add an equals after the
argument to support this. Otherwise it won't complete after an equals. But this also makes Zsh
complete the option with an equals. It seems inconsistent currently, with somea arguments
using an equals and some not, but maybe there was some rationale behind it. I'm not sure what
other Zsh completion scripts generally do about this. What should be done? Iv'e kept it the
same in Bash, except there it always completes both after an equals and in a a separate
argument so it's not stricly required to add an equals sign to long options there it's just a
matter of style if we prefer to complete with an equals or not.

Not sure what this means, let's just push it and we'll hear people scream, if it's not good enough.

Better completion for -g/--goto in Bash?

It's fine.

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 13, 2018

Let's try a GitHub mention...

@Mellbourn @okapia This PR to VS Code adds a Zsh completion script which is a modified version of the one in the Zsh distribution to be bundled and installed together with VS Code. That way it can be updated alongside updates to VS Code rather than having to wait for a Zsh update in your distro or otherwise compiling Zsh yourself. But we need to be sure with you that you are ok with this licensing wise since I'm no lawyer and I don't really know how the Zsh license applies for copying and modifying a single file, so I'm asking instead. Feel free to look at this PR anyhow 馃槂

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 13, 2018

Do you prefer rebase or merge to fix the conflict?

@Mellbourn

This comment has been minimized.

Copy link

Mellbourn commented Sep 13, 2018

Hi!
I think it is a good idea to try to package command line completion with code (I've suggested so myself a while back).

I am personally fine with my original zsh code having inspired parts of this code. However, I'm not a lawyer either, so I don't really know the rules here.

Concerning the zsh code, I made some minor improvements in this commit. Specifically,

  • --folder-uri parameter with slightly different documentation (I know zsh maintainers prefer the wording "specified" for parameter arguments).
  • I made --disable-extension and --disable-extensions exclusive since it does not make sense having them both in the same command line.

What do you think should happen to the completion code in the zsh disribution? Should it be removed?

@okapia

This comment has been minimized.

Copy link

okapia commented Sep 13, 2018

@Mellbourn contributed it to zsh and nobody else has changed it since so the copyright belongs to him. So either he need to agree to licence it under the VS code licence or you need to include the zsh licence in a comment in the file header.

@Mellbourn

This comment has been minimized.

Copy link

Mellbourn commented Sep 13, 2018

I am absolutely willing to sign the VS code Contributor License Agreement. However, I'm unsure of exactly how to do that, since I don't have a pending pull request. Please advice.

@okapia

This comment has been minimized.

Copy link

okapia commented Sep 13, 2018

code, like most commands, allows long option arguments to be after an equals or in the next
argument. In Zsh completion's _arguments function you need to add an equals after the
argument to support this. Otherwise it won't complete after an equals. But this also makes Zsh
complete the option with an equals. It seems inconsistent currently, with somea arguments

Normally we specify it to match whatever the command accepts. Any inconsistency would imply the command itself is inconsistent:

'--option[description]:arg'     # arg always follows option in next argument
'--option=[description]:arg'   # either form is valid
'--option=-[description]:arg'  # the equals is required

The form with an equals implies that either form is accepted by the command. Zsh will then complete the equals by default. If all of code's options accept an equals then that is the form that should be used.

@Mellbourn

This comment has been minimized.

Copy link

Mellbourn commented Sep 13, 2018

@okapia gave a lot of constructive comments to my original pull request, so I would recommend anyone interested in the motives for why the script looks the way it does, to read through those comments: zsh-users/zsh#24

@okapia

This comment has been minimized.

Copy link

okapia commented Sep 13, 2018

With regard to the changes relative to Klas Mellbourn's zsh function, it seems that you have mostly reverted the descriptions to closer match those from code --help. Not all are improvements - I'd revert most of them but not all - removing full stops is in line with zsh convention for example. In some cases, the changes would actually be improvements if applied to the --help output.

As Klas mentioned we tend to like the word "specify" at the start of descriptions for options that take an argument. "set" and "give" can be fine too. It ends up being clearer with all descriptions starting with a verb and common verbs helping to group options. Your change of "opens" to "open" and "uploads" to "upload" is an improvement, however. For both consistency and brevity we typically stick to the shorter verb form without the 's'.

Your change to --max-memory with regard to specifying the units goes against zsh convention. We leave it out of the option description - it is irrelevant at that point - and include it in the argument description in parentheses.

The worst bit of wording which also comes from the --help output is "force to open a new window.", and similarly for the following option. This is bad English because there is no subject in the sentence - force what? It is better worded as just "open a new window" , or perhaps add the word "always" or "unconditionally". Wording it as a command gives you something that is both valid and which tends tro be short. I'd be interested to know if this phrase was written by an American. I see that construction a lot from German colleagues so it tends to bother me.

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 13, 2018

Seems mentioning you sparked your interest 馃槃

Hi!
I think it is a good idea to try to package command line completion with code (I've suggested so myself a while back).

I am personally fine with my original zsh code having inspired parts of this code. However, I'm not a lawyer either, so I don't really know the rules here.

Concerning the zsh code, I made some minor improvements in this commit. Specifically,

* `--folder-uri`  parameter with slightly different documentation (I know zsh maintainers prefer the wording "specified" for parameter arguments).

* I made `--disable-extension` and `--disable-extensions` exclusive since it does not make sense having them both in the same command line.

What do you think should happen to the completion code in the zsh disribution? Should it be removed?

I tried to merge these changes in. See below about the help string change. No idea what should happen to the version in the Zsh distribution...

code, like most commands, allows long option arguments to be after an equals or in the next
argument. In Zsh completion's _arguments function you need to add an equals after the
argument to support this. Otherwise it won't complete after an equals. But this also makes Zsh
complete the option with an equals. It seems inconsistent currently, with somea arguments

Normally we specify it to match whatever the command accepts. Any inconsistency would imply the command itself is inconsistent:

'--option[description]:arg'     # arg always follows option in next argument
'--option=[description]:arg'   # either form is valid
'--option=-[description]:arg'  # the equals is required

The form with an equals implies that either form is accepted by the command. Zsh will then complete the equals by default. If all of code's options accept an equals then that is the form that should be used.

Than I guess I should change all long options, taking an argument, to use the equals form since VS Code uses minimist which accepts either form. I wonder if I should do this for the Bash completion script too? So the completion feels the same between them.

With regard to the changes relative to Klas Mellbourn's zsh function, it seems that you have mostly reverted the descriptions to closer match those from code --help. Not all are improvements - I'd revert most of them but not all - removing full stops is in line with zsh convention for example. In some cases, the changes would actually be improvements if applied to the --help output.

As Klas mentioned we tend to like the word "specify" at the start of descriptions for options that take an argument. "set" and "give" can be fine too. It ends up being clearer with all descriptions starting with a verb and common verbs helping to group options. Your change of "opens" to "open" and "uploads" to "upload" is an improvement, however. For both consistency and brevity we typically stick to the shorter verb form without the 's'.

Your change to --max-memory with regard to specifying the units goes against zsh convention. We leave it out of the option description - it is irrelevant at that point - and include it in the argument description in parentheses.

The worst bit of wording which also comes from the --help output is "force to open a new window.", and similarly for the following option. This is bad English because there is no subject in the sentence - force what? It is better worded as just "open a new window" , or perhaps add the word "always" or "unconditionally". Wording it as a command gives you something that is both valid and which tends tro be short. I'd be interested to know if this phrase was written by an American. I see that construction a lot from German colleagues so it tends to bother me.

I tend to agree with what you are saying about the help strings. I reverted the help strings since I felt like it makes sense for it to be as close as possible to the output of VS Code's --help if it's a completion script that VS Code itself bundles, with the expectation that it is more likely that they be kept in sync this way. The trade off being, of course, that some of the strings are not perfect, and are not aligned with the Zsh completion scripts that are bundled with the Zsh distribution.

I wonder how we should go about this:

  1. It can be merged with the current help strings, leaving it to a separate PR to improve the help strings, probably also changing src/vs/platform/environment/node/argv.ts. This can give you a chance to make a PR to VS Code if you want to.
  2. I can try to merge the modified ones from the original _code completion script with the changes that I made that are good. Whether src/vs/platform/environment/node/argv.ts should than be modified in this PR, a separate PR, or not at all, is a different matter.

@joaomoreno @Mellbourn @okapia what do you think?

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Sep 14, 2018

Guys this is starting to escalate. It should be a fairly simple PR. If it's tackling too many things, let's break it apart and solve this for each shell independently.

We should also not diverge from the help strings that are today in Code. If the phrases are not proper English, let's fix them in their original places first.

It would also be great if we didn't have to duplicate these things between the actual CLI implementation and the several completion contributions. I'm always much more in favour of a single edit point, from which both parts would take the information out.


On another note, I was not aware that zsh already ships code completions. Why should we ship this, then? I am getting more and more convinced that this should go into external projects.

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 14, 2018

Guys this is starting to escalate. It should be a fairly simple PR. If it's tackling too many things, let's break it apart and solve this for each shell independently.

It's still just as simple, all of this is really about polishing the completion scripts to get it just right.

The only thing that is going out of scope here is the help strings thing. @Mellbourn @okapia Changed the help strings in the Zsh completion scripts from the ones in the VS Code --help message, and I changed them back since I thought that if VS Code is going to be supplying the completions it will be weird for there to be one help message in the --help and a different one in the completion script, unlike in the case of completions that are supplied with Zsh where it makes sense for them to use Zsh's conventions. See what I suggested above:

  1. It can be merged with the current help strings, leaving it to a separate PR to improve the help strings, probably also changing src/vs/platform/environment/node/argv.ts. This can give @Mellbourn a chance to make a PR to VS Code if he wants to.
  2. I can try to merge the modified ones from the original _code completion script with the changes that I made that are good. Whether src/vs/platform/environment/node/argv.ts should than be modified in this PR, a separate PR, or not at all, is a different matter.

I was not aware that zsh already ships code completions. Why should we ship this, then? I am getting more and more convinced that this should go into external projects.

The advantage to shipping completion scripts with your product, rather than leaving it to others, are that they can be kept in-sync with changes to it, which is useful for software that is installed from outside a distro's package repository. For example in the case of the Zsh completion script, it will only be updated once the distro updates Zsh, which only happens on a new version of the distro in most cases. The same applies to bash_completions.

Many pieces of software already maintain and bundle their own completion scripts. Sometimes even software that is included in distros, which have their completion scripts packaged into the deb/rpm/etc.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Sep 14, 2018

If we're going to improve the product strings, I'd prefer that to go in a separate PR.

Also, could we make those completions generated out of our sources instead of duplicating the strings?

@Mellbourn

This comment has been minimized.

Copy link

Mellbourn commented Sep 14, 2018

A question: would this activate the zsh completions for a homebrew installation of code on macOS too? (the completion included in zsh is activated automatically for all OSs)
(If not, I still think it's a good idea to bundle completion with code, but then this feature should be added at some point)

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Sep 15, 2018

Also, could we make those completions generated out of our sources instead of duplicating the strings?

Probably, but that can get complicated, getting the subtle details of the completion script right is hard enough as it is..

A question: would this activate the zsh completions for a homebrew installation of code on macOS too? (the completion included in zsh is activated automatically for all OSs)
(If not, I still think it's a good idea to bundle completion with code, but then this feature should be added at some point)

I don't own a Mac. I made it include the completion scripts under the VS Code resources directory for all operating systems. To get this to work will probably mean that the homebrew packaging will have to copy/symlink them to the correct location. Doubt there is any support in the normal Mac application packages for installing completions which is how VS Code is officially packaged.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Sep 17, 2018

Doubt there is any support in the normal Mac application packages for installing completions which is how VS Code is officially packaged.

We can have an F1 command which installs them, just like we have to install the command line launcher.

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Oct 7, 2018

We can have an F1 command which installs them, just like we have to install the command line launcher.

Sounds like that will be the right thing to do. We just need to remember to open an issue to add this once this is merged. (I won't be able to test if I code this myself since I don't own a Mac)

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Jan 4, 2019

Merging this and closing #56497. Let's tackle each shell/feature in separate issues as users request them.

@joaomoreno joaomoreno merged commit a9e56b8 into Microsoft:master Jan 4, 2019

2 checks passed

VS Code #20180922.7 succeeded with issues
Details
license/cla All CLA requirements met.
Details

@segevfiner segevfiner deleted the segevfiner:shell-completions branch Jan 4, 2019

@segevfiner segevfiner referenced this pull request Jan 4, 2019

Open

Remaining tweaks to Zsh completion script #66039

0 of 3 tasks complete
@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 7, 2019

@joaomoreno filter this out on Windows maybe?

image

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Jan 7, 2019

@bpasero They are useful on Windows too. Be it Cygwin, MSYS2, Bash on Ubuntu on Windows (Or whatever it's marketed as now). In the future, maybe even Powershell completions will be added there.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 7, 2019

@segevfiner how do shells know where to look for completions, is this a standard?

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Jan 7, 2019

@segevfiner how do shells know where to look for completions, is this a standard?

They look by default at places relative to their installation prefix (From configure/autoconf) e.g. /usr/share/zsh/site-functions if the installation prefix is /usr. The exact directory can be overridden/configured, so it depends on the installed shell/OS. Even if not found automatically by the shell, simply including those in the distribution allows users to symlink them in order to load them into their shell. That way they also update alongside VS Code. (And they weigh like 4KB total anyhow)...

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 7, 2019

@segevfiner yeah I was wondering why this has to be a folder outside our app folder to begin with

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Jan 7, 2019

@bpasero Makes a good point, since in Windows it doesn't make sense to have them there. Thinking about it, these files only make sense on installers (deb, rpm).

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Jan 7, 2019

They are useful for Windows users too, as I explained above...

I placed it under resources to make it more discoverable by the user.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Jan 7, 2019

Yes, but how will they get discoverable by Cygwin, MSYS2, etc?

@segevfiner

This comment has been minimized.

Copy link
Contributor Author

segevfiner commented Jan 7, 2019

They won't be discovered automatically of course. But the user can symlink to the scripts installed under VS Code's install dir, with the benefit that the scripts will auto update alongside VS Code. It's quite convenient that way, and it's how some other software I remember using handled this, though Indon't remember which off the top of my head.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 7, 2019

@segevfiner Can this installation issue be explained by this change somehow? I never had it before and it seems to hint at completions:

image

(this is installing code-exploration deb package on Ubuntu 18)

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Jan 7, 2019

Yup. These need to be scoped. I've commented them out for now and created an issue to follow up: #66154

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 7, 2019

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment