Skip to content

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?

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

@azdanov
Copy link

azdanov commented Jun 24, 2025

Hey. I found out about this PR from quarkusio/quarkus#22710. I guess that currently it's out of date and would need to be rebased with main and fix any potential issues?

@maxandersen
Copy link
Contributor

i've rebased this PR with main in #2463

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.

4 participants