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

[WIP] Fish completion #725 #1925

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

Conversation

volkov
Copy link

@volkov volkov commented Jan 15, 2023

Fish completion POC (#725). Very dirty and buggy at the moment, but works nice for my scenario.

Definitely, it's early for reviewing of this changes, but early adopters of this thing are welcome.

@volkov volkov force-pushed the feature/fish-completion branch from 3af1c4b to 96c3ec2 Compare January 15, 2023 19:09
@volkov volkov force-pushed the feature/fish-completion branch from 96c3ec2 to f5de8e3 Compare January 15, 2023 19:33
@remkop remkop added this to the 4.8 milestone Jan 16, 2023
@remkop remkop added theme: auto-completion An issue or change related to auto-completion theme: codegen An issue or change related to the picocli-codegen module labels Jan 16, 2023
@volkov volkov force-pushed the feature/fish-completion branch from f5de8e3 to 1af8274 Compare January 16, 2023 05:10
@remkop
Copy link
Owner

remkop commented Jan 17, 2023

Awesome, @volkov thank you for working on this! 👍
Will you be able to provide some unit tests?

@volkov
Copy link
Author

volkov commented Jan 22, 2023

Yes, I have begun to move towards testing, however, my progress is currently moderate as I am not familiar with dejagnu
I think I'll be back in couple of weeks.

@volkov volkov force-pushed the feature/fish-completion branch 3 times, most recently from cbb3219 to 5e210f0 Compare January 22, 2023 18:42
@remkop
Copy link
Owner

remkop commented Feb 4, 2023

Hi @volkov looks like you have made great progress! I took a brief look and I see you are also adding dejaGnu tests, much appreciated! I remember it took me a long time to wrap my head around the dejaGnu test framework, not trivial! 😅

Were you able to make dejaGnu start a fish shell session instead of a bash session? I took a quick look at src/test/dejagnu.tests/lib/library.exp (I suspect that some work may be needed there) but I haven't figured it out yet. Looking at the start_bash function, it may be as simple as setting --tool_exec to fish, but perhaps that is not even needed, not sure...

If your dejaGnu tests work, can we call them from JUnit, maybe from src/test/java/picocli/AutoCompleteDejaGnuTest.java?

Again, thanks a lot for your contributions here! Much appreciated! 🙏

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

I found some usages of Java 8 API, but picocli is compiled with Java 5, so we cannot use that (String.join). Can you take a look?

src/main/java/picocli/AutoComplete.java Outdated Show resolved Hide resolved
src/main/java/picocli/AutoComplete.java Outdated Show resolved Hide resolved
src/main/java/picocli/AutoComplete.java Outdated Show resolved Hide resolved
@volkov
Copy link
Author

volkov commented Feb 8, 2023

Were you able to make dejaGnu start a fish shell session instead of a bash session?

Yes, At the moment I use separate file dejagnu.fishtests/lib/completion.exp with exp_spawn fish --no-config

If your dejaGnu tests work, can we call them from JUnit, maybe from src/test/java/picocli/AutoCompleteDejaGnuTest.java?

Fish tests work on my notebook, at the moment there is only subset of bash tests. Bash tests for some reason don't work on my notebook, but I didn't investigate it yet.

Yes, It's good idea to integrate them with junit.

Also I think its important to make them work in github actions.

I'll try to return to this pr in couple of weeks.

@volkov volkov force-pushed the feature/fish-completion branch from 5b1cbc8 to a144222 Compare February 24, 2023 13:30
@volkov volkov force-pushed the feature/fish-completion branch from 331f26e to 4b7483f Compare February 25, 2023 13:26
@volkov volkov force-pushed the feature/fish-completion branch from 4b7483f to 811f4df Compare February 25, 2023 13:32
@volkov volkov force-pushed the feature/fish-completion branch from 5078561 to bb4d76a Compare February 25, 2023 13:46
@volkov
Copy link
Author

volkov commented May 8, 2023

Currently tests on github are flaky, don't know why (locally they are quite stable).

I want to investigate what's wrong, but I don't know when.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: auto-completion An issue or change related to auto-completion theme: codegen An issue or change related to the picocli-codegen module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants