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.

@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
@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
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.

None yet

2 participants