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

Feat: Add zsh, fish and PowerShell completion support #8122

Merged
merged 3 commits into from
Feb 4, 2021
Merged

Feat: Add zsh, fish and PowerShell completion support #8122

merged 3 commits into from
Feb 4, 2021

Conversation

benmezger
Copy link
Contributor

@benmezger benmezger commented Jan 6, 2021

This PR adds support for zsh and fish completion.

See issue #4296

@bep bep requested a review from anthonyfok January 7, 2021 09:16
@bep
Copy link
Member

bep commented Jan 7, 2021

Looks good to me -- I added @anthonyfok as a reviewer, as I remember vaguely that he was "talking about this" at some point.

@bep
Copy link
Member

bep commented Jan 29, 2021

Bump, I would love to see this merged.

/cc @anthonyfok

@benmezger I assume you have confirmed that this actually work?

Copy link
Member

@anthonyfok anthonyfok left a comment

Choose a reason for hiding this comment

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

Hello! The patch itself looks good, but probably more work is needed, in separate commits.

  • Zsh completion works. I placed the generated file in "${fpath[1]}/_hugo" (according to https://github.com/spf13/cobra/blob/master/shell_completions.md), i.e. /usr/local/share/zsh/site-functions/_hugo on my system.

  • Fish completion probably works, but somehow fish already comes with its own hugo completion in /usr/share/fish/completions/hugo.fish, and for some reasons completion still works after removing that file.

What needs to be done, probably as separate commits:

  1. PowerShell completion does not work due to old version of Cobra being listed in go.mod.
    PowerShell support was added to Cobra on 2020-12-29, but the latest released version of Cobra v1.1.1 is dated October 2020, thus PowerShell completion is not working (fish completion is output instead.), so probably Cobra needs a new release, and Hugo's go.mod needs to be updated accordingly.

  2. hugo gen autocomplete writes to /etc/bash_completion.d/hugo.sh by default regardless of shell --type unless --completionfile is defined.
    I have always uneasy on this behaviour (which dates way back to the early days of Hugo) especially /etc is not supposed to be writeable by normal (non-admin) users. But yes, writing zsh/fish/PowerShell completion file as /etc/bash_completion.d/hugo.sh is definitely incorrect, so this needs to be fixed somehow, probably in a separate PR/commit.

@anthonyfok
Copy link
Member

If the two problems I listed above are better served by separate pull requests, I can change the review stations from "requested changes" to "approved". Please let me know. :-)

@benmezger
Copy link
Contributor Author

@anthonyfok, thanks for the input, I have some questions regarding the issues you mentioned.

  1. From what I understood regarding powershell, is that we should drop powershell support for now, is this correct?
  2. I agree with /etc not being ideal. What I think it would be a solution is to print the completion to stdout by default, and the user may pipe to a file or specify --completionfile instead, but, I would like to hear more suggestions, as the path to completion file really depends on the user setup/os.

@anthonyfok
Copy link
Member

@anthonyfok, thanks for the input, I have some questions regarding the issues you mentioned.

  1. From what I understood regarding powershell, is that we should drop powershell support for now, is this correct?

No. I think keeping PowerShell support is good. What needs to be done is that we should release Cobra v1.1.2 or 1.2.0, and have Hugo depend on the new Cobra version, so the PowerShell completion generation would work. I will look into it.

  1. I agree with /etc not being ideal. What I think it would be a solution is to print the completion to stdout by default, and the user may pipe to a file or specify --completionfile instead, but, I would like to hear more suggestions, as the path to completion file really depends on the user setup/os.

I agree: printing to stdout is a saner default. It might break some existing packaging scripts, but I doubt it: besides Debian's debian/rules which I maintain and already specifies --completionfile, Hugo's Homebrew also specifies --completionfile too https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/hugo.rb

So, yes, please feel free to go ahead with making this change!

@bep
Copy link
Member

bep commented Feb 1, 2021

I suggest we take the Powershell thing as a separate ... thing.

@anthonyfok
Copy link
Member

I was wrong... even with the latest Cobra (master, or "github.com/spf13/cobra v1.1.2-0.20210126175524-9df156e6d11e"), PowerShell autocompletion generation still doesn't work: it is still generating a fish file instead on my end.

And actually Cobra v1.1.1 does have PowerShell support; the latest commits after v1.1.1 merely enhances it. My previous hypothesis was all wrong. Sorry!

@bep, @benmezger, is PowerShell completion working for you at all?

I suggest we take the Powershell thing as a separate ... thing.

Good idea! I didn't even know that Cobra has support for PowerShell completion, haha!

@anthonyfok anthonyfok changed the title Feat: Add zsh, fish and powershell completion support Feat: Add zsh and fish completion support Feb 2, 2021
@benmezger
Copy link
Contributor Author

@anthonyfok I've made the suggested changes. If you have any suggestion on handling stdout files instead of the if check I've made, let me know, regarding @bep suggestion on removing powershell, I've completely removed powershell for now.

@benmezger benmezger requested a review from anthonyfok February 2, 2021 23:32
@bep
Copy link
Member

bep commented Feb 3, 2021

@anthonyfok feel free to merge, it looks good to me. I'm not a big Powershell (or Windows user; I have an old Windows 8.1 VM that I test Hugo stuff on (only version I have license for :-), not sure it even has Powershell...).

@benmezger a quick note: the doc below /commands gets autogenerated so any manual change to it will be overwritten. It does not matter too much here, but I'm just mentioning it.

Copy link
Member

@anthonyfok anthonyfok left a comment

Choose a reason for hiding this comment

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

Looks good to me, and all tests work out! I love how it ouputs to stdout by default.
Thank you @benmezger for the great work!

@anthonyfok anthonyfok merged commit d36fd5b into gohugoio:master Feb 4, 2021
@benmezger benmezger deleted the feat/add-multiple-shell-support branch February 5, 2021 14:41
@bep
Copy link
Member

bep commented Feb 6, 2021

@anthonyfok for the future it would be good if you could squash commits like these into one (it's one logical unit).

@anthonyfok
Copy link
Member

@anthonyfok for the future it would be good if you could squash commits like these into one (it's one logical unit).

@bep Indeed! Thank you for the advice! I noticed the "3 separate commits that should have been one" too late, and wished I could have gone back and fixed it, and yet the "squash commit" option didn't even cross my mind! My inexperience. Will definitely be more careful in future PRs. Thank you!

@benmezger
Copy link
Contributor Author

benmezger commented Feb 9, 2021

@bep @anthonyfok I should have noticed it too. It didn't occur to me that the CONTRIBUTION guideline actually details this.
My bad peeps – will pay attention to this on a next contribution.

@anthonyfok anthonyfok changed the title Feat: Add zsh and fish completion support Feat: Add zsh, fish and powershell completion support Feb 9, 2021
@anthonyfok anthonyfok changed the title Feat: Add zsh, fish and powershell completion support Feat: Add zsh, fish and PowerShell completion support Feb 9, 2021
anthonyfok added a commit that referenced this pull request Feb 9, 2021
Revert "Refactor: Remove powershell support" with fixes

Thanks to Ben Mezger (@benmezger) for the original code.
See #8122

This reverts commit a7c515e.
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants