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

Allow users of picocli-shell-jline3 to use clear command and a nicer help command #1265

Merged
merged 9 commits into from
Dec 14, 2020
Merged

Conversation

sualeh
Copy link
Contributor

@sualeh sualeh commented Nov 23, 2020

Allow users of picocli-shell-jline3 to use clear command and a nicer help command.

  • The clear screen command is provided out of the box by jline2, and similarly should be built into picocli.shell.jline3.PicocliCommands. This change allows users of PicocliCommands to include a clear screen command with a single line: picocliCommands.includeClearScreenCommand(reader); - see the example.
  • The help command is provided by jline3 by default, but picocli help is much nicer. This change allows users of PicocliCommands to override the jline3 help command with a single line: systemRegistry.register("help", picocliCommands); - see the example.

@sualeh sualeh changed the title Allow users of picocli-shell-jline3 to use clear command and a nicer `help command Allow users of picocli-shell-jline3 to use clear command and a nicer help command Nov 23, 2020
@sualeh
Copy link
Contributor Author

sualeh commented Nov 25, 2020

@remkop - any thoughts on this?

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.

@sualeh I finally had time to look at this in more detail.

I have an idea for keeping things more declarative, can you take a look and let me know your thoughts?

@codecov-io
Copy link

codecov-io commented Nov 27, 2020

Codecov Report

Merging #1265 (c026772) into master (e5033e1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1265   +/-   ##
=========================================
  Coverage     93.82%   93.82%           
  Complexity      462      462           
=========================================
  Files             2        2           
  Lines          6852     6852           
  Branches       1842     1842           
=========================================
  Hits           6429     6429           
  Misses          138      138           
  Partials        285      285           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5033e1...a0db671. Read the comment docs.

@sualeh
Copy link
Contributor Author

sualeh commented Dec 1, 2020

@remkop - thanks for the last snippet. I think I have finally understood (sorry for being dense), and pushed what I think are the final changes. Please take a look. Thanks.

@remkop
Copy link
Owner

remkop commented Dec 1, 2020

Great! Let me check when I get to my PC.

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.

Thank you for the changes, @sualeh, this looks pretty good now!

For finishing touches, can you add some javadoc?
Also I believe the factory and the clear-screen subcommand should require a LineReaderImpl; this is not great, but we cannot call clearScreen on any LineReader. This is also something we should probably raise with the JLine maintainers: they should consider adding the clearScreen method to the LineReader interface.

2. Use LineReaderImpl instead of LineReader, and have the client/ caller do the cast
3. Add a constructor to allow factories to be chained
Copy link
Contributor Author

@sualeh sualeh left a comment

Choose a reason for hiding this comment

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

@remkop - I have made the clean-up changes you suggested.

  • I want to remove workDir from PicocliCommands. Please let me know if you need a separate pull request for that, or if I can include it in the same PR.
  • I will test the suggestion in Expose clearScreen on LineReader jline/jline3#610 and see if that works - then we can go back to using LineReader

@remkop
Copy link
Owner

remkop commented Dec 1, 2020

I'm fine with removing workDir (thanks! :-)), no need for a separate pull request.

Happy to use the LineReader interface if possible, let me know how it goes.

@sualeh
Copy link
Contributor Author

sualeh commented Dec 1, 2020

@remkop - I think we are done. I have removed workDir, and the clear screen is not working - see my comments on jline/jline3#610

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.

This looks good, thank you!

I will merge this soon.
This will be included in the upcoming picocli 4.6 release.

Thank you again for the contribution!

@remkop remkop added this to the 4.6 milestone Dec 2, 2020
@remkop remkop added type: API 🔌 type: enhancement ✨ theme: integration An issue or change related to integration with other frameworks, shells or operating systems theme: shell An issue or change related to interactive (JLine) applications labels Dec 2, 2020
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.

Ah very nice!
Using a public API like Terminal is a lot nicer than having to use internal API like LineReaderImpl.
Thanks for following up!

@remkop remkop merged commit 9c929a1 into remkop:master Dec 14, 2020
@remkop
Copy link
Owner

remkop commented Dec 14, 2020

Merged.
Thank you again for the contribution!

MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: integration An issue or change related to integration with other frameworks, shells or operating systems theme: shell An issue or change related to interactive (JLine) applications type: API 🔌 type: enhancement ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants