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

completion: pip: Only invoke pip when trying to complete it #1834

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

NoahGorny
Copy link
Member

Description

Invokes pip only when actively trying to complete it.

Motivation and Context

This should greatly reduce shell load time, as the pip invocation takes
a lot of time.

How Has This Been Tested?

Locally

Types of changes

  • 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 change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@NoahGorny
Copy link
Member Author

@cornfeedhobo @davidpfarrell @nwinkler what is your opinion on this?
it greatly improves shell load time for me

@cornfeedhobo
Copy link
Member

@NoahGorny I'd love to test this more thoroughly soon, but I'm a little bogged down. If you're okay waiting through this week, I'll probably get to it.

Copy link
Member

@cornfeedhobo cornfeedhobo left a comment

Choose a reason for hiding this comment

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

Tested. It's beautiful. Let's definitely get this out.

Copy link
Member

@cornfeedhobo cornfeedhobo left a comment

Choose a reason for hiding this comment

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

Looks great and tests great, but small change requested. Thanks!

completion/available/pip.completion.bash Outdated Show resolved Hide resolved
completion/available/pip.completion.bash Outdated Show resolved Hide resolved
completion/available/pip3.completion.bash Outdated Show resolved Hide resolved
completion/available/pip3.completion.bash Outdated Show resolved Hide resolved
@cornfeedhobo
Copy link
Member

I'm also thinking we might want to generalize the lazy loading of any function, but I think I could handle that in a follow up PR.

This should greatly reduce shell load time, as the pip invocation takes
a lot of time
@NoahGorny
Copy link
Member Author

@cornfeedhobo finally got around to actaully doing it, thanks for the CR 😄

@hongyi-zhao
Copy link
Contributor

@cornfeedhobo finally got around to actaully doing it, thanks for the CR smile

What's the meaning of CR used by you here?

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

First I was worried about the surprised delay the user might get on first use, but TIL that bash-completion loads completions lazily by default.

We might consider a framework to also lazy-load completions by default, but until then, this seems legit.

@davidpfarrell
Copy link
Contributor

@hongyi-zhao wrote:

What's the meaning of CR used by you here?

I believe @NoahGorny uses CR ubiquitously to mean "Code Review"

@NoahGorny
Copy link
Member Author

@cornfeedhobo ping 😄

Copy link
Member

@cornfeedhobo cornfeedhobo left a comment

Choose a reason for hiding this comment

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

This looks great. There is a small lag on the first tab, but that's to be expected with lazy loading.

@NoahGorny NoahGorny merged commit 4984d7a into Bash-it:master Sep 20, 2021
@NoahGorny NoahGorny deleted the lazily-load-pip branch September 20, 2021 11:47
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