search

kislyuk / argcomplete

Python and tab completion, better together.
https://kislyuk.github.io/argcomplete/
Apache License 2.0
1.39k stars 129 forks source link

Support of path completion in fish #327 #359

Closed volkov closed 2 years ago

volkov commented 2 years ago

First version of absolute path completion for fish. #327 fix

volkov commented 2 years ago

@kislyuk @evanunderscore take a look please, I think it could be merged.

evanunderscore commented 2 years ago

Looks good to me. Are you able to add a test case to the test suite that verifies this? It could probably go in the base class since I'd expect this to work for Bash too.

kislyuk commented 2 years ago

@volkov let us know if you need help adding a test.

volkov commented 2 years ago

Thank you @volkov. As @evanunderscore said, this looks good but needs a regression test to make sure the new behavior is covered by the test suite.

Ok, I will add first version of test to discuss.

volkov commented 2 years ago

I have added first version of test https://github.com/kislyuk/argcomplete/pull/359/files#diff-ae070fbd66c55f7bf108994c0a42ffe69eee7319b5bf0f47bef0fcdbd8b496b3R1385

It could probably go in the base class since I'd expect this to work for Bash too.

I think its better to write separate base test for path completion because setUp is different:

volkov commented 2 years ago

Looks like bash don't resolve relative paths, same test for bash fails on /prog basic f\t

I can split test in two and disable relative path test for bash, or leave only fish test.

evanunderscore commented 2 years ago

Is it actually necessary to have a separate class for these tests? Having the test directory on the path shouldn't prevent you from trying to complete an absolute or relative path. If for some reason it is actually necessary to do that, I'd prefer to have the common logic in a base class rather than duplicating it.

I think it makes sense to split the absolute and relative tests into two test functions.

volkov commented 2 years ago

Is it actually necessary to have a separate class for these tests?

Directory on the path is fine, but I can't test path completion when command completion registered. When completion registered for command prog fish would trigger it also for ./prog and for test/prog. So I can't create test that would pass with both registered completions, but fail with only command completion.

I'd prefer to have the common logic in a base class rather than duplicating it. I think it makes sense to split the absolute and relative tests into two test functions.

I'll try.

volkov commented 2 years ago

So I can't create test that would pass with both registered completions, but fail with only command completion.

Actually it's possible if I have two scripts with different names, e.g prog and frog - first with command completion, second with path, but I don't want to duplicate test script.

kislyuk commented 2 years ago

LGTM, thanks for your patience @volkov.