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

Document System.exit usage #1857

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Document System.exit usage #1857

merged 3 commits into from
Dec 20, 2022

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Oct 23, 2022

Documentation for a usecase not to use System.exit.

Resolves #1855

Resolves #1855

Signed-off-by: Tadaya Tsuyukubo <tadaya@ttddyy.net>
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 made several suggestions for changes. Can you take a look?

docs/index.adoc Outdated Show resolved Hide resolved
docs/index.adoc Outdated Show resolved Hide resolved
docs/index.adoc Show resolved Hide resolved
docs/index.adoc Outdated
The `System.exit` call finishes maven process bypassing the rest of the maven steps including its error handlings.

In picocli, any exceptions thrown during the `CommandLine.execute` method are captured, printed, and converted to an exit code.
You can check the exit code and perform an appropriate action, such as throwing an exception, instead of calling `System.exit`.
Copy link
Owner

Choose a reason for hiding this comment

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

This looks strange: you are correct that picocli captures any exceptions thrown during the CommandLine.execute method. That is the difference with the CommandLine.parseArgs method. If an application wants to throw an exception when a problem occurs, it should just use parseArgs.

It would be strange to first capture an exception, convert to an error code, and then use that error code to throw another, less informative, exception...
The manual should not recommend that but instead should recommend using parseArgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to recommend parseArgs and linked to the 9.7. DIY Command Execution section

docs/index.adoc Outdated Show resolved Hide resolved
@remkop
Copy link
Owner

remkop commented Nov 3, 2022

Thank you!
I’m offline until mid next week, so feedback will take a while, thank you for your patience.

@remkop remkop added this to the 4.7.1 milestone Dec 20, 2022
@remkop remkop merged commit 3e50eb6 into remkop:main Dec 20, 2022
@remkop
Copy link
Owner

remkop commented Dec 20, 2022

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document a pattern that do not return exit code
2 participants