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

lakectl autocomplete with repository name #4320

Merged
merged 5 commits into from
Oct 9, 2022

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Oct 5, 2022

Enhance the auto-complete with auto complete by listing the current repositories.
This is the first step in enable better user-experience while working from the command line.
Next step will be to enable specific tag/branch/commit and path.

Part of #654

@nopcoder nopcoder added the include-changelog PR description should be included in next release changelog label Oct 5, 2022
@nopcoder nopcoder requested a review from johnnyaug October 5, 2022 22:04
@nopcoder nopcoder self-assigned this Oct 5, 2022
@nopcoder nopcoder linked an issue Oct 8, 2022 that may be closed by this pull request
Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

  1. Prefix extraction seems broken: I think all repositories are always fetched.
  2. Couldn't get it to work on my machine (Mac + Zsh).

// extract repository name written so far
var prefix api.PaginationPrefix
if strings.HasPrefix(toComplete, uriPrefix) {
idx := strings.Index(toComplete[len(uriPrefix):], uri.PathSeparator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the previous if entail that idx is always -1 here?

At this point, I think you should take everything after the uriPrefix as the prefix for the query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, fixing.

@nopcoder
Copy link
Contributor Author

nopcoder commented Oct 9, 2022

  1. Prefix extraction seems broken: I think all repositories are always fetched.
  2. Couldn't get it to work on my machine (Mac + Zsh).

Thanks for the review - fixing the prefix use. if you like to get it working on your mac, try: lakectl completion zsh > "${fpath[1]}/_lakectl"

@johnnyaug
Copy link
Contributor

@nopcoder worked! Thanks

@nopcoder
Copy link
Contributor Author

nopcoder commented Oct 9, 2022

@nopcoder worked! Thanks

Pushed the fix - now the repository prefix passed to to the api call.

@nopcoder nopcoder requested a review from johnnyaug October 9, 2022 09:36

// extract repository name written so far
var prefix api.PaginationPrefix
if strings.HasPrefix(toComplete, uriPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it doesn't have the lakefs:// prefix, we still want an autocomplete, no?

So maybe the prefix should be "toComplete".
For example, if I'm doing lakectl branch create my-rep<TAB>, I want to get: lakectl branch create lakefs://my-repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to give the same autocomplete behaviour we have with other application - when the prefix you enter doesn't match any of the possible options we do not suggest completion.
Example: docker autocompete will match the prefix to the image name - but if you will find me a other working reference I will consider :)

Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

Nice feature!! See another comment

@nopcoder nopcoder merged commit f212cad into master Oct 9, 2022
@nopcoder nopcoder deleted the feature/cli-complete-first-repo branch October 9, 2022 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakectl autocomplete nouns
2 participants