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

[FEATURE] Autocompletion #4709

Closed
wants to merge 21 commits into from
Closed

Conversation

lazybensch
Copy link
Contributor

check out the RFC to learn more about this feature.

things left to be done:

  • make CI green
  • apply code review comments
  • add fish support
  • decide on cache invalidation technique
  • re-source shell config file

to entertain you until then:

@@ -4,6 +4,9 @@
// Provide a title to the process in `ps`
process.title = 'ember';

var autocomplete = require('../lib/utilities/cli-command-completion');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this instead be run in cli/index.js ?

changes in bin/ember come at a big cost, as bin/ember is meant to work across all versions of ember-cli. In all but the most extreme cases there is a moratorium on changes to it.

@stefanpenner
Copy link
Contributor

awesome

break;
case 'zsh':
fs.appendFileSync(initFile, template('. <(' + this.program + ' --completion)'));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a fish user I would love to have autocomplete support for it as well. If you don't have time, I can investigate this myself at a later date.

@lazybensch lazybensch force-pushed the autocompletion branch 4 times, most recently from 0e04c93 to 65b519a Compare August 20, 2015 07:57
@lazybensch lazybensch force-pushed the autocompletion branch 4 times, most recently from ca9fb60 to e65c58f Compare August 20, 2015 11:40
@stavarotti
Copy link
Contributor

👏 This looks awesome.

@lazybensch lazybensch force-pushed the autocompletion branch 9 times, most recently from ae8a163 to 2429991 Compare September 3, 2015 19:47
@lazybensch lazybensch force-pushed the autocompletion branch 6 times, most recently from a8c220e to 623b1fc Compare September 5, 2015 13:11
@stefanpenner
Copy link
Contributor

@kellyselden @lazybensch whats the word on this.

@lazybensch
Copy link
Contributor Author

@stefanpenner i am still trying to get the specs green. (it may not seem like it though, since i am always amending to my last commit)

Feature works fine on my machine and all but I really have a hard time getting the CI happy. On travis, now there only are a couple of tests red on NODE=0.10 for whatever reason ^^ 0.12 and io.js is green however.

windows is another story. I don't have a windows machine to test on so i am currently using appveyor to test my changes. thats a frustratingly slow process :(

@Turbo87
Copy link
Member

Turbo87 commented Mar 15, 2016

@lazybensch can you explain why you introduced the CliCommand class? does that offer anything that the Command class isn't offering?

also why did you introduce another JSON manifest format thingy instead of reusing the output of ember help --json? I could imagine that if that command was cached the same way that you proposed it would have other benefits (e.g. for IDE integration).

@knownasilya
Copy link
Contributor

Ilya Radchenko even though every command you execute is invalidating the cache anyways?

This way you could create a postinstall npm script that invalidates the cache, and you never have to deal with the install issue.

ember serve takes a while, and using ember help or something clutters your CLI and seems like a hack.

@lazybensch
Copy link
Contributor Author

@knownasilya wouldn't a postinstall only invalidate the cache on the contributor machine that is adding an addon via ember install addon-with-commands? when a collaborator does git pull; npm i he will have an invalid cache, right?

@knownasilya
Copy link
Contributor

No, because postinstall runs after any npm install in that project, but the consumer has to create the postinstall on their end (in their ember app package.json). If the project already has it defined in the package.json, a contributor can pull down and npm install, and the postinstall will run for them.

@lazybensch
Copy link
Contributor Author

@knownasilya well thx for the suggestion then. I'll implement it the way you described.

@Turbo87 a CliCommand is, despite the similar name, a semantically different thing (for example do also blueprints need a CliCommand representation). This Class describes the actual words that are displayed on the command line and what words should follow etc.

The help --json was not implemented back when i first started this PR. It's current output has some format properties that do not play along with command line completion and of course its generation takes to long. I can, however make the help --json command read the cache file instead (i think this is what you proposed in the second half of your comment).

I have not touched this so far because, well... you see this feature is way more involved as i first anticipated and i would have prefered to "not care" about this for now, get a mergeable MVP, and DRY-ing up the json generation in a later PR but just now as i wrote this thought down i already realize that this is not a good idea :D

also i've red IDE integration a couple of times in context of help --json, what is that about? can you give me an example?

@knownasilya
Copy link
Contributor

It might even make sense to add the postinstall to new projects via the app blueprint in this PR.

@Turbo87
Copy link
Member

Turbo87 commented Mar 15, 2016

also i've red IDE integration a couple of times in context of help --json, what is that about? can you give me an example?

take https://github.com/Turbo87/intellij-emberjs for example. it allows you to run ember generate from the UI, but it currently has a hardcoded list of available blueprints. if there was something like a <project>/node_modules/ember-cli/.help.json the plugin could replace the hardcoded list with the blueprints that are actually supported in the project similar to how the autocompletion would show a list of supported blueprints.

The help --json was not implemented back when i first started this PR.

good point. but it is implemented now, so IMHO we should try to reuse what already exists.

It's current output has some format properties that do not play along with command line completion

can you give examples of what isn't playing along?

and of course its generation takes to long

thus my suggestion to cache the output similar to what you're suggesting in this PR

I can, however make the help --json command read the cache file instead (i think this is what you proposed in the second half of your comment).

I actually would like to propose the opposite: cache the output of help --json and use that for the autocompletion instead of introducing another JSON commands cache file format

@lazybensch
Copy link
Contributor Author

well, here are a few arguments why i would prefere using the cli-command-generator:

  • When the help --json code was written, it kind of made sense to put it inside lib/commands/help.js since it was directly related to this command. Now that it would generate a cache file that is both used for autocompletion and help output (especially now that i know about IDE integration) it should be decoupled from the help command and move into a seperate module. Decoupling the currently existing json generation from the help.js would be quite some effort. Especially considering that i already provide this with lib/models/cli-command-generator.js which sole responsible is to generate that json.

  • As I mentioned there are a few inconsistencies in the way json needs to be formatted for either the help output or the cli-completion. For example shell completion does not discriminate between blueprints and commands or native commands and addon commands. (I mean of course this can be added as meta information but should not effect the structure of the command tree). I believe IDE integration would also profit from the structure used for autocompletion.

  • This is quite a weak argument but still: cli-command-generator is fully documented

  • The JSON generated by help --json does not mention that you can do ember help generate model. However this information needs to be represented when you want a capable autocompletion. Again, i believe IDE integration would profit from this aswell.

  • To future proof this feature it is important to have a good API for commands and blueprints (build-in
    ones and those of addon authors) the cliCommands function that you can override in your command or blueprint passes you, in case of autocompletion, what cliCommands are already typed (example below). This might not seem like much at first glance but it means that addon authors will have partial control over how there commands should be completed. For example the help command overrides cliCommands to see if it was already mentioned like so:

      cliCommands: function(generator, parentCommands) {
        if (parentCommands.indexOf('help') > -1) {
          return [];
        } else {
          return generator.commands;
        }

    The effect is that when you do ember <tab> your shell will list help, among others, as a suggestion, and if you do ember help <tab> it will again, suggest help (since ember help help is a valid command) but once you do ember help help or ember help generate you will not get help as a suggestion anymore.

I believe the last point is bigger then it seems. Because addon authors are able to help users with suggestions.For example imagine ember-deploy would be a monolithic addon (instead of using plugins) that would let you deploy to any destination. The addon introduces a ember deploy command, you invoke it by passing a destination. The addon author could now override the cliCommands function of his deploy method to suggest ['s3', 'redis', 'heroku']. A user could then do ember deploy <tab> and gets all suggestions, or simply do ember deploy he<tab> to safe time. Another usecase is that you can easily add deeply nested commands to your addon if you want to.

I think at this point it also becomes clear why a cliCommand is not a command, despite being a blueprint, you could add "autocomplete suggestions" for your command, those would technically only by arguments to your command but still work with shell completion.

@Turbo87
Copy link
Member

Turbo87 commented Mar 16, 2016

Decoupling the currently existing json generation from the help.js would be quite some effort.

I totally agree on that, but I still think it's worth the effort. The alternative of having essentially two JSON outputs (the ember help --json one and yours) feels kinda dirty and confusing to me.

For example shell completion does not discriminate between blueprints and commands or native commands and addon commands.

I think it should be reasonably easy to convert the current help --json output to the command tree that you're proposing.

To future proof this feature it is important to have a good API for commands and blueprints (build-in ones and those of addon authors) the cliCommands function that you can override in your command or blueprint

correct me if I'm wrong, but I think we could still do the same thing if we used the other JSON output. we would need to figure out if the last "cli command" was a command or blueprint, look it up and then add the cliCommands() result to the list of completions.

@lazybensch
Copy link
Contributor Author

I hope i am not wearing you out with this discussion but there is one point i think i haven't made clear:
I don't want to have two JSON generations, i too definitely only want to keep one. So naturally there are two options:

option A

  • remove JSON generation in command/help.js
  • replace it with with a function that maps my cached json into the help format
  • profit

option B

  • remove JSON generation in `cli-command-generator.js``
  • make the completer map the help --json cached structure into autocomplete format
  • refactor the code in commands/help.js into its own module
  • make it possible to control subcommands based on already typed commands
  • make the help command use this api to reproduce the behavior i explained earlier
  • document that new module
  • profit

Not only is option B a lot more effort on my side to have the same result; I argue that option A and option B have the exact same result. Actually if i squash my commits i bet you could not tell which option i chose as following option B would result in a decoupled documented module that supports the features i mentioned. But that is exactly what i already have implemented in cli-command-generator.js.

@Turbo87
Copy link
Member

Turbo87 commented Mar 16, 2016

I guess it boils down to the question which of the formats is more generic and can be used by more than just the autocompletion code. I would personally answer this question with the ember help --json output because I think it better represents the internal structure, explicitly mentions the addons, etc.

@lazybensch
Copy link
Contributor Author

I still feel adding those properties to the autocompletion would be less effort then adding the missing properties to the help code. also i am not quite sure if i can produce the structure needed for autocompletion out of the help-json but think the other way around is not that hard (as i said, i would add addon and blueprint information as meta data to all the cliCommands in my hash

@homu
Copy link
Contributor

homu commented Mar 16, 2016

☔ The latest upstream changes (presumably #5612) made this pull request unmergeable. Please resolve the merge conflicts.

@nathanhammond
Copy link
Contributor

Added to tomorrow's ember-cli core team meeting agenda.

@Turbo87
Copy link
Member

Turbo87 commented Mar 20, 2016

@lazybensch can you change this PR to use the json-generator utility for now until #5635 is decided upon and merged? I'd rather have slow completion than no completion at all 😉

@sobi3ch
Copy link

sobi3ch commented Sep 8, 2016

So what's happening here, is this issue going anywhere?

@Turbo87
Copy link
Member

Turbo87 commented Feb 16, 2019

closing this for now due to missing activity. would love it if someone would pick it up again though.

@Turbo87 Turbo87 closed this Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants