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

cli: completion: Add support for Nushell completions #3073

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Feb 16, 2024

Incidentally, I also fixed outdated docs in the command help for other shells (still using --<shell> instead of the positional form)

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/) (command documentation)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Dependencies

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LGTM, could you please adhere to our commit style too, like cli: and lib:?

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Feb 16, 2024

And it seems someone forgot to update the snapshot of the cli-reference.

@poliorcetics
Copy link
Contributor Author

@PhilipMetzger done to both !

@thoughtpolice
Copy link
Member

thoughtpolice commented Feb 17, 2024

Note that in #2353 this was also attempted, but due to a bug in clap_complete_nushell it actually generates invalid nushell code. I assume your upstream commit fixes this behavior? If so it might make sense to mark the upstream clap bug/PR as completed as well as the earlier one here.

@poliorcetics poliorcetics force-pushed the ab/push-vtvmulzoskqm branch 2 times, most recently from fee6052 to 4851997 Compare February 17, 2024 18:37
@poliorcetics poliorcetics marked this pull request as ready for review February 17, 2024 18:37
@poliorcetics
Copy link
Contributor Author

I assume your upstream commit fixes this behavior

Yes, and it's been released with 4.5.1, so this PR is now ready. Thanks for pointing out the existing issues and PRs :)

@PhilipMetzger
Copy link
Contributor

Can you add a Closes #2323 to the commit message or the PR description? Then it should be good to land.

@poliorcetics
Copy link
Contributor Author

Yes done

@poliorcetics poliorcetics changed the title feat(completion): Add support for Nushell completions cli: completion: Add support for Nushell completions Feb 18, 2024
Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

lgtm 👍🏻

@poliorcetics
Copy link
Contributor Author

Rebased on top of #3062 so I can finally merge them sequentially without them interfering with each other in the changelog

@poliorcetics poliorcetics enabled auto-merge (rebase) February 18, 2024 18:01
@poliorcetics poliorcetics merged commit c752307 into jj-vcs:main Feb 18, 2024
15 checks passed
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.

FR: nushell completion support
3 participants