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 shell auto-completion #1603

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

Add shell auto-completion #1603

wants to merge 54 commits into from

Conversation

hoonman
Copy link

@hoonman hoonman commented Aug 5, 2024

  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Contributors: @Tienbruh, @JesusG2022, @hoonman

Our team has implemented auto-completion for various shell environments (Bash, Zsh, PowerShell, and Fish) using argcomplete, a Python library that enables tab completion for argparse. We have integrated argcomplete into the _cli.py file, updated the pyproject.toml file to include argcomplete, and revised the README.md to provide instructions on activating the tab completion feature.

Additional context & links

This addresses #843

hoonman and others added 28 commits July 31, 2024 22:23
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Added argcomplete to the dependencies
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Added argcomplete to _cli.py
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Added bash command requirement for bash in the readme file
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
simplified step to register argcomplete
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Added activation instructions for zsh and fish to readme
Modified readme to include both temporary and permanent autocomplete
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
removed global completion and added subsections for different shells
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Reworded CLI Usage section
Co-authored-by: hoonman <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
Merge OpenAI main with our fork
Co-authored-by: Tienbruh <[email protected]>
Co-authored-by: JesusG2022 <[email protected]>
@hoonman hoonman marked this pull request as ready for review August 6, 2024 20:08
@hoonman hoonman requested a review from a team as a code owner August 6, 2024 20:08
Copy link
Collaborator

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

pyproject.toml Outdated
@@ -17,6 +17,7 @@ dependencies = [
"cached-property; python_version < '3.8'",
"tqdm > 4",
"jiter>=0.4.0, <1",
"argcomplete >= 1.12.0"
Copy link
Collaborator

@RobertCraigie RobertCraigie Sep 3, 2024

Choose a reason for hiding this comment

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

I'd like to avoid adding a new required dependency, can we add this as an optional dependency?

I'm thinking something like this, thoughts?

pip install openai[cli]

Copy link
Author

@hoonman hoonman Sep 3, 2024

Choose a reason for hiding this comment

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

Thank you for the feedback! Our team discussed this and wanted to share our approach to add argcomplete as an optional dependency.

To avoid making argcomplete a required dependency, we plan to:

  1. Change argcomplete from a required to an optional dependency in pyproject.toml.
  2. Add a proxy file for argcomplete to handle optional imports.
  3. Update the __init__.py file in the extras directory to reflect these changes.
  4. Modify the README to instruct users to use the [cli] option to install argcomplete.

Please let us know if there are any additional changes we should consider.

Copy link
Collaborator

@RobertCraigie RobertCraigie Sep 3, 2024

Choose a reason for hiding this comment

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

That sounds good to me! I don't think there's any other changes needed.

For what it's worth I don't think we need the complexity of a proxy here though as the dependency is only ever used in one place anyway we can just change that call site to try ... except ImportError

Copy link
Author

Choose a reason for hiding this comment

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

We have included the changes for making argcomplete an optional dependency, the try...except ImportError to the _cli.py, as well as the pip install openai[cli] command in the README. Let me know if there's anything else that needs modification. Thank you!

pip install openai[cli]
```

### Bash & Zsh
Copy link
Collaborator

@RobertCraigie RobertCraigie Sep 10, 2024

Choose a reason for hiding this comment

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

question: are there existing docs we could link to here instead of listing out all the possible environments here? maybe argcomplete's docs?

Choose a reason for hiding this comment

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

Both temporary and permanent argcomplete activation instructions for Fish and Powershell are available on argcomplete's contrib directory. However, only the temporary activation commands for Bash and Zsh are listed on argcomplete's README file. The commands to activate autocompletion permanently for Bash and Zsh are not found anywhere on argcomplete's docs.

What are your opinions on replacing all the other environment activation instructions with the appropriate links to argcomplete's docs, while keeping these lines for permanent activation on Bash and Zsh?

For Bash:

register-python-argcomplete openai >> ~/.bashrc

For Zsh:

register-python-argcomplete openai >> ~/.zshrc

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.

4 participants