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: auto inject completions #139

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Conversation

35C4n0r
Copy link
Contributor

@35C4n0r 35C4n0r commented Mar 8, 2024

Pull Request Title

Automate Shell Completion Configuration

Description

Adds automatic injection of the autocomplete script into the appropriate shell profile and then instructing the user on how to source the file (or instruct to open a new terminal).

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Related Issue(s)

This PR addresses issue #49
Closes #49
/claim #49

Screenshots

Notes

Please add any relevant notes if necessary.

@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 8, 2024

@Tpuljak PR is ready for review.

@Tpuljak
Copy link
Member

Tpuljak commented Mar 9, 2024

@35C4n0r the changes regarding the autocompletion injection look great.

Before I approve, please remove the contributors commit from this PR and add it as a separate PR (example).

@35C4n0r 35C4n0r force-pushed the feat-autocompletions branch from 9f153c5 to dd9aa56 Compare March 9, 2024 10:49
@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 9, 2024

@Tpuljak removed that commit.

@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 9, 2024

@Tpuljak contributors PR here #150

pkg/cmd/autocomplete.go Outdated Show resolved Hide resolved
@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 9, 2024

@Tpuljak pushed the changes that fixes that issue 👍🏻

@35C4n0r 35C4n0r requested a review from Tpuljak March 9, 2024 11:30
pkg/cmd/autocomplete.go Outdated Show resolved Hide resolved
pkg/cmd/autocomplete.go Outdated Show resolved Hide resolved
@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 9, 2024

@Tpuljak made the required changes.
PS: sorry for making you review so many times 😅

@35C4n0r 35C4n0r requested a review from Tpuljak March 9, 2024 12:05
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

The code is now inline without codebase and the command does update the profile with the completion script. Good work!

The issue now is that the completion script does not seem correct.

For me, the completion script just autocompletes paths when called.

For example:

daytona c + TAB does not suggest daytona create but simply fills in a directory that starts with c.

If it works for you as intended, let me know. If not, please look into this.

@idagelic might be useful in this conversation. He implemented completions for one of our other CLI tools.

@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 9, 2024

@Tpuljak It works fine for Bash + Fish + ZSH (Manual + new shell).

@Tpuljak
Copy link
Member

Tpuljak commented Mar 9, 2024

@35C4n0r you're right. I can confirm that the completions work.
Screenshot 2024-03-09 at 13 32 23

The issue I mentioned above was related to my Warp terminal. All good. Approving the PR.

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Awesome work!

@Tpuljak Tpuljak requested a review from idagelic March 9, 2024 12:35
@idagelic
Copy link
Member

This looks good, thanks for the PR @35C4n0r !

The Cobra default "completion" command prints out the completion script results to stdout. Users might expect this behavior and use it for some custom shell profile configs. Should we leave this option to the user but perhaps just hide it from help to avoid confusion using spf13/cobra#1541 ?

In that case there would be no unexpected behavior to the user and we could rename this new command to "autocomplete" just as the go file is called. Does this make sense to you? @Tpuljak

@35C4n0r
Copy link
Contributor Author

35C4n0r commented Mar 11, 2024

@idagelic made the required changes.

Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

Thank you @35C4n0r

@idagelic idagelic merged commit a870ad8 into daytonaio:main Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate Shell Completion Configuration for daytona to Enhance User Experience
3 participants