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

Links shell completion files on install #575

Closed
wants to merge 1 commit into from

Conversation

jbnicolai
Copy link
Contributor

Includes shell-completion and will run, on postinstall, link bash completion/bash and `link zsh completion/zsh.

This will, if possible, link both the bash and zsh completion files to their expected locations.

Could use some feedback on the linking of zshell's completion, as I don't use that personally.

edit:

Removed zsh linking for now, best looked at someone actually using it.

@jbnicolai jbnicolai closed this Jul 6, 2014
@jbnicolai jbnicolai reopened this Jul 6, 2014
@phated
Copy link
Member

phated commented Jul 6, 2014

@jbnicolai
Copy link
Contributor Author

@phated It does work when run in or from zsh, but still requires bash to be installed (as it is on nearly every system.

Let me however see if I can make the script posix shell compliant, so it just uses /bin/sh and doesn't require bash :)

@@ -56,7 +57,8 @@
"prepublish": "marked-man --name gulp docs/CLI.md > gulp.1",
"lint": "jshint lib bin index.js --reporter node_modules/jshint-stylish/stylish.js --exclude node_modules",
"test": "npm run-script lint && mocha --reporter spec",
"coveralls": "istanbul cover _mocha --report lcovonly -- -R spec && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage"
"coveralls": "istanbul cover _mocha --report lcovonly -- -R spec && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage",
"postinstall": "./node_modules/shell-completion/bin/link bash completion/bash || exit 0"
Copy link
Contributor

Choose a reason for hiding this comment

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

npm adds bins to the path, so you can just do:

link bash completion/bash

@sindresorhus
Copy link
Contributor

@jbnicolai why isn't link just in node?

@jbnicolai
Copy link
Contributor Author

Shell seemed easier when I started, thinking "it's just setting a symlink". Then it grew, of course.. ;-)

I'll close this PR for now and rewrite it in node tomorrow - as I agree that that would be better.
Thanks for the feedback, @sindresorhus and @phated

@jbnicolai jbnicolai closed this Jul 6, 2014
@sindresorhus
Copy link
Contributor

@jbnicolai hah, always :)

Powershell support would also be nice.

@jbnicolai jbnicolai reopened this Jul 13, 2014
@yocontra
Copy link
Member

@tkellen liftoff?

@jbnicolai
Copy link
Contributor Author

Reopened the pull request - rewrote shell-completion in node for cross-platform portability 😄

Powershell support should be trivial, but I'm completely unfamiliar with the platform and am unsure what the correct installation path would be. Some help on this would be greatly appreciated!

CC: @sindresorhus @phated

@jbnicolai
Copy link
Contributor Author

@tkellen, @contra indeed! :)

@sindresorhus
Copy link
Contributor

@staxmanade did the current PowerShell autocomplete.

@sindresorhus
Copy link
Contributor

@jbnicolai, @contra meant that it would probably be a better fit in liftoff which is what gulp uses for the CLI stuff.

@jbnicolai
Copy link
Contributor Author

Ah. mumbles something about miscommunication and the internet.. 😉

I think it does belong in gulp though, since the completion files themselves are part of gulp's repository as well. @tkellen might decide to bundle it with liftoff in the future, which he would be more than welcome to, in order for more CLI tools to use it - but I don't see how it's part of the same project per se.

@yocontra
Copy link
Member

I think that bundling the autocompletion into liftoff would be a better fit though IMO, they all do the same thing - run a command which returns a list of things then make it work with zsh/powershell/sh so its really just boilerplate around a command that liftoff could take care of

@sindresorhus
Copy link
Contributor

I tend to agree with @contra. If it's possible it would be better to have it in liftoff so anyone using it would get autocompletion for free.

@tkellen
Copy link

tkellen commented Jul 14, 2014

Agreed about adding this to Liftoff--I'm happy to consider the addition. Feel free to open a PR over there, @jbnicolai. I'll try to look at it this week. Thanks for putting this together!

@jbnicolai
Copy link
Contributor Author

👍 I'm not very familiar with liftoff, but I'll try to put together a PR :)

Thanks for the feedback, guys!

@jbnicolai jbnicolai closed this Jul 14, 2014
@tkellen
Copy link

tkellen commented Sep 16, 2014

@jbnicolai any traction on getting a PR together for Liftoff?

@yocontra
Copy link
Member

Any progress here? @jbnicolai @tkellen

@tkellen
Copy link

tkellen commented Dec 1, 2014

@jbnicolai Your module is a great start! I think it needs some work to be general cased in this fashion, though. Before I just write the module I'm looking for, would you be open to refactoring yours?

In my mind, the ideal case would be having your library export a pair of methods something like this:

// dynamically generate the content of a completion file
generateCompletion({
  command: 'gulp --simple-tasks',
  type: 'bash'
});

// write completion file
installCompletion({
  name: 'gulp',
  content: 'the output of generateCompletion',
  type: 'bash'
});

Once that is available, tools like gulp can expose your functionality via whatever command they like. For example, gulp --install-completions could defer to your library (or possibly some code in Liftoff) which handles the orchestration for consumers.

What do you think?

@jbnicolai
Copy link
Contributor Author

@tkellen I'd very much be open to refactoring/improving the module! What I have now is nothing but a quick draft, and due to some personal issues I haven't had the time I wanted to work on these kinds of outstanding issues.

Your suggestion looks great though. Would you be interested in implementing this in the current module? I'd be happy to give you access, and share npmjs.org ownership.

@tkellen
Copy link

tkellen commented Dec 1, 2014

Hey @jbnicolai, I probably won't have time until January but if you want to add me to the repo, that'd be great!

@yocontra
Copy link
Member

yocontra commented Dec 1, 2014

@tkellen @jbnicolai Just let me know when you have resolved this and what changes need to be put in gulp to accommodate the new feature in liftoff 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants