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

_brew | allow 'brew commands' to be cached by zsh #5388

Merged
merged 1 commit into from Dec 10, 2018
Merged

_brew | allow 'brew commands' to be cached by zsh #5388

merged 1 commit into from Dec 10, 2018

Conversation

bosr
Copy link
Contributor

@bosr bosr commented Dec 8, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Hi,

Here is an update to _brew where the most expensive command (__brew_all_commands) has its results cached. This results in faster (immediate?) completion. This should help people with slow machine (my mac at work is hugely slowed down by the security measures).

I tried my best and used the standard mechanism; the code follows along the many available examples.

The user will have its results cached if his/her shell is configured to do so: meaning that the standard zstyle (use-cache) has to be set to 'yes' and that the cache will be saved by default in ~/.zcompcache/brew_all_commands (also configurable using the zstyle cache-path). The delay for cache invalidation is set to 24h completions/zsh/_brew#135. I expect no kitten to be harmed with this PR.

Regarding tests, I only conducted manual checks:

  • with no existing cache file at ~/.zcompcache/brew_all_commands: the cache gets created
  • with an invalid (>24h) cache file: the cache gets recreated
  • with a valid cache file (<=24h): the cache is simply used and thus will eventually get older than 24h.

I hope you'll review it and it eventually gets accepted.
Have a great day.

@MikeMcQuaid
Copy link
Member

Nice work, this seems pretty neat. Would it be worth similarly caching the names of formulae or is that pretty fast?

CC @apjanke who often has ZSH opinions.

I tried my best and used the standard mechanism; the code follows along the many available examples.

Could you post some links here for our learning?

Thanks and great work!

@bosr
Copy link
Contributor Author

bosr commented Dec 9, 2018

Hi Mike, thanks for the feedback :) Here are some references and impl details. Cheers!

Some references

Here is a good example of zsh completion involving the caching mechanism, I used it as my master example: zsh-completions/src/_pygmentize, along with other completions from zchee/zsh-completions.

For going further, the Zsh reference manual for completion is chapter 20. In particular the functions used here are all described: _retrieve_cache, _cache_invalid, _store_cache, and the zstyles use-cache, cache-path, cache-policy. It is famously difficult to digest, I think it's because of the many cross-dependencies between the concepts. I spent some time recently and finally start to get more familiar with it... until I forget it all again! This docset proved to be useful when navigating the doc.

Implementation detail

The main difference with my code is in the if statements, for instance this one at line 86:

_get_formatters() {
    # ...
    if ( [[ ${+_pygmentize_formatter} -eq 0 ]] || _cache_invalid pygmentize_formatter ) \
          && ! _retrieve_cache pygmentize_formatter; then
        # recreate cache
    fi
    # ...
}

When _get_formatters is invoked, the global variable (_pygmentize_formatter) may be not null, so their tests make sense and the "semantics" are correct.

In my code for the function __brew_all_commands, because I did not want to change the way it is currently factored, the array commands is local to the function and thus gets created on each invocation. So I had to drop the first test ${+...} -eq 0 in order to let a chance to the _cache_invalid mechanism a chance to execute, and maintain the same "semantics".

@bosr
Copy link
Contributor Author

bosr commented Dec 9, 2018

Regarding formulae name caching, good catch, let's try it!

@apjanke
Copy link
Contributor

apjanke commented Dec 10, 2018

This sounds like a good idea, and as far as I can tell, looks like idiomatic ZSH code.

@MikeMcQuaid MikeMcQuaid merged commit 00075c2 into Homebrew:master Dec 10, 2018
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @bosr!

@bosr bosr deleted the zsh/completion-cache branch December 10, 2018 21:30
@lock lock bot added the outdated PR was locked due to age label Jan 9, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants