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

zsh completion... again #565

Merged
merged 3 commits into from
Jul 26, 2017

Conversation

okdana
Copy link
Contributor

@okdana okdana commented Jul 26, 2017

Sorry for PR spam, found another issue with this. :/

I hadn't noticed that the old completion function didn't support completing values without a space after short options — doing something like rg -C3 <tab> instead of rg -C 3 <tab> would break the completion.

I fixed that. And, as discussed last time, i updated the test stuff so that it just pulls the optspecs straight out of the function. That should make the 'parsing' a lot more reliable (at the expense of requiring zsh for the test, obv).

Not sure if i put the zsh-installation stuff in the right place, guess we'll see what it says...

@okdana
Copy link
Contributor Author

okdana commented Jul 26, 2017

Oh, i guess Travis doesn't actually use that arch. I looked for the addons.apt.packages file it mentioned but i don't see it. I don't know much about Travis unfortunately :/

Also i guess i should make it so that script.sh skips the test if zsh isn't installed? Rather than just exploding?

@BurntSushi
Copy link
Owner

No worries about the PRs! This is kinda how I expect this sort of thing to go. Iterate and improve!

With respect to Travis, I think you can install zsh by putting a special incantation into the .travis.yml. Like this: https://docs.travis-ci.com/user/installing-dependencies/#Installing-Packages-on-Standard-Infrastructure

Also i guess i should make it so that script.sh skips the test if zsh isn't installed? Rather than just exploding?

I think exploding is fine actually. That way, if zsh is somehow not installed, then we aren't tricked into believing the test is running and passing. If we want to test somewhere that zsh isn't feasible to install, then we should specifically ignore the test on that configuration.

ci/install.sh Outdated
@@ -45,11 +45,23 @@ EOF
fi
}

install_zsh() {
case $TARGET in
aarch64-unknown-linux-gnu)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think I understand this. Why are you trying to install zsh only when testing on aarch64? aarch64 is ARM. I suspect ripgrep probably works on ARM, but I don't run CI on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i kind of just copy-and-pasted the section above it without thinking too much about it. In hind-sight i see that it special-cased ARM because the C tool chain is arch-specific. But since you mentioned you didn't write these scripts i assume even that is probably just boiler-plate.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see now. Yeah, all boiler plate. The original source is rust-everywhere.

@okdana okdana force-pushed the bug/zsh-completion-again branch from b3f42bb to c7f0800 Compare July 26, 2017 13:00
@okdana
Copy link
Contributor Author

okdana commented Jul 26, 2017

I reverted the install.sh stuff and made it explode again if zsh is missing. I added the zsh package to .travis.yml per your link; that seems to sort it for Linux. Darwin doesn't need anything special because it ships with zsh. Looks like it worked!

@okdana
Copy link
Contributor Author

okdana commented Jul 26, 2017

Oh, it doesn't like pipe_fail though. Not really necessary, will remove and rebase.

@okdana okdana force-pushed the bug/zsh-completion-again branch from c7f0800 to 9057895 Compare July 26, 2017 13:16
@okdana
Copy link
Contributor Author

okdana commented Jul 26, 2017

Looks OK now i think

@BurntSushi
Copy link
Owner

Awesome, looks great! Thanks so much!

@BurntSushi BurntSushi merged commit d4b790f into BurntSushi:master Jul 26, 2017
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.

2 participants