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: check for completion files #618

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

Conversation

vladdoster
Copy link
Member

@vladdoster vladdoster commented Jan 9, 2024

Description

Related Issue(s)

Motivation and Context

Usage examples

before

zinit for cp"hub-*/etc/hub.zsh_completion -> _hub" from'gh-r' sbin'hub' @mislav/hub

after

zinit for from'gh-r' sbin'hub' @mislav/hub

How Has This Been Tested?

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation change
  • New feature (non-breaking change which adds functionality)

Checklist:

  • All new and existing tests passed.
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.

@vladdoster
Copy link
Member Author

Automatically detect completion files

Screenshot 2024-01-09 at 05 38 15

@vladdoster
Copy link
Member Author

@jankatins, thoughts?

@vladdoster vladdoster self-assigned this Jan 14, 2024
local c cfile bkpfile
# The plugin == . is a semi-hack/trick to handle 'creinstall .' properly
nt () {
Copy link
Contributor

Choose a reason for hiding this comment

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

whats nt?

Copy link
Member Author

@vladdoster vladdoster Jan 18, 2024

Choose a reason for hiding this comment

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

nt is just a random function name. It could be anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, stupid question: why not a speaking function name so one can actually know what it does by just reading the function name? reading the zinit code is hard enough even without random function names :-)

Copy link
Member Author

@vladdoster vladdoster Jan 18, 2024

Choose a reason for hiding this comment

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

@jankatins,

lol, I just took the name from the zsh docs. But agreed, it should be renamed.

Screenshot 2024-01-18 at 1 55 51 PM

@jankatins
Copy link
Contributor

jankatins commented Jan 15, 2024

To be honest, I'm fine with the "less magic" variant of renaming to _something instead of automagically figuring out a file which contains completions ("explicit is better than implicit" -> python zen :-)). I tried to figure out which line actually contains the trigger and I think I found it (**/(_*|*.zsh*) or so) and that looks fine...

What happens if two files end up getting symlinked under the same name?

assert $state equals 0
assert "$ZPLUGINS/test---ignored_completions/_valid" is_file
assert "$ZPLUGINS/test---ignored_completions/__init__.py" is_file
assert "$ZINIT[COMPLETIONS_DIR]/_valid" is_file
COMPS=( "$ZINIT[COMPLETIONS_DIR]"/_* )
assert __init__.py is_not_value_in $COMPS
zinit delete --yes test/ignored_completions
run zinit cuninstall test/ignored_completions
assert $output contains 'No completions found for `test/ignored_completions'
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a test with two files of the same name getting linked under different names.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jankatins Good idea.

Signed-off-by: Vladislav Doster <mvdoster@gmail.com>
Signed-off-by: Vladislav Doster <mvdoster@gmail.com>


- name: "gh-r"
run: $HOME/.local/bin/zunit run --verbose tests/gh-r.zunit
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: $HOME/.local/bin/zunit run --verbose tests/gh-r.zunit
run: zunit run --verbose tests/gh-r.zunit

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.

3 participants