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

Add completion subcommand #1968

Merged
merged 3 commits into from
Aug 22, 2022
Merged

Add completion subcommand #1968

merged 3 commits into from
Aug 22, 2022

Conversation

tranzystorekk
Copy link
Contributor

There seem to have been a few attempts at completions in the past, but generating from a subcommand seems to be the most versatile for both users and package maintainers. Maybe the old manually written completion scripts could be ditched as well for simplicity?

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

@Keats
Copy link
Collaborator

Keats commented Aug 16, 2022

Honestly I'm kind of wondering if it's even worth adding considering how few commands zola has.

@tranzystorekk
Copy link
Contributor Author

IMO even two commands is a good reason to have them, and zola serve in particular has quite a few flags that also get completed.

@c-git
Copy link
Contributor

c-git commented Aug 16, 2022

If I may be permitted to add my opinion. It is always wonderful if a program supports completions. It's not required but a pleasant surprise if you find out it's available. If there are no material downsides I would say this is a nice thing to add. Are there any notable disadvantages to having this added? I did note that some things needed to be commented out but I didn't quite understand what they were.

PS. I've really come to appreciate how careful @Keats is with regard to what is added. I can't tell for sure but it makes it seem like you have a vision for the project and you try to keep it on that vision while being open to other people's ideas. You never just shoot things down without a reason. Even like this you gave your reasoning why you are reluctant. Kudos to you.

Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

I'll try locally later to see how it works in practice

src/main.rs Outdated Show resolved Hide resolved
@tranzystorekk
Copy link
Contributor Author

tranzystorekk commented Aug 17, 2022

If we merge this, I'd point it out in the changelog that the scripts in completion/ are obsolete so the package maintainers know what to use, or even remove that whole dir in this PR

@Keats
Copy link
Collaborator

Keats commented Aug 17, 2022

Let's remove them in that PR, they haven't been updated in years anyway.

@Keats Keats merged commit 923e21f into getzola:next Aug 22, 2022
@Keats
Copy link
Collaborator

Keats commented Aug 22, 2022

Thanks!

@tranzystorekk tranzystorekk deleted the completion branch August 22, 2022 21:22
Keats pushed a commit that referenced this pull request Feb 16, 2023
* Add completion subcommand

* Remove old completion scripts

* Move completion logic
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.

3 participants