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

Add --failure-level command line option to force non-zero exit code (fixes #1113) #1114

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Conversation

pasieronen
Copy link
Contributor

Kind of change

  • Bug fix
  • New non-breaking feature
  • New breaking feature
  • Documentation update
  • Build improvement

Description

The Ruby asciidoc CLI has option --failure-level which forces the CLI to exit with non-zero status code if the specified logging level (severity) is reached. See: asciidoctor/asciidoctor#2674, asciidoctor/asciidoctor#2003, asciidoctor/asciidoctor#854

This PR implements the same functionality for AsciidoctorJ, pretty much the same way as the Ruby code (keeps track of highest seen log message severity, and checks that the end).

The default value is the same as in Ruby - only "fatal" severity causes non-zero exit code - which probably won't break many things (but someone running AsciidoctorJ as part of a CI build would probably want to fail on "error" severity as well, maybe even "warn").

Issue

Fixes #1113

…rom CLI if specified logging level is reached
Copy link
Member

@robertpanzer robertpanzer 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 your PR!
This looks great!
I added a few comments. What do you think about them?

public class SeverityConverter implements IStringConverter<Severity> {

@Override
public Severity convert(String argument) { return Severity.valueOf(argument.toUpperCase()); }
Copy link
Member

Choose a reason for hiding this comment

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

When I pass an invalid option like INFOX I currently get a stacktrace back that does not show the currently supported options:

Exception in thread "main" java.lang.IllegalArgumentException: No enum constant org.asciidoctor.log.Severity.INFOX
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.asciidoctor.log.Severity.valueOf(Severity.java:3)
	at org.asciidoctor.jruby.cli.SeverityConverter.convert(SeverityConverter.java:10)
...

What do you think about changing it so that you'll get just a message with the invalid value and all supported values like:

Could not convert severity 'INFOX'. Supported values are [DEBUG, INFO, WARN, ERROR, FATAL, UNKNOWN]

Nitpick: Could you please add newlines after the opening brace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - and it turns out JCommander has EnumConverter class which gets us this nice error message for free.

@@ -507,8 +510,15 @@ private RubyClass getExtensionGroupClass() {

@Override
public void log(LogRecord logRecord) {
if (this.maxSeverity.compareTo(logRecord.getSeverity()) < 0) {
this.maxSeverity = logRecord.getSeverity();
Copy link
Member

Choose a reason for hiding this comment

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

JRubyAsciidoctor can be used in a multithreaded way when embedded into other environments like Spring Boot apps or sth similar.
In such scenarios it would be dangerous to rely on this property.
Could this be moved into a custom org.asciidoctor.log.LogHandler that is only added by the AsciidoctorInvoker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, moved this to MaxSeverityLogHandler under org.asciidoctor.jruby.cli package.

@robertpanzer
Copy link
Member

Thanks for the PR! 🎉
I'll take care of porting this into the 2.5.x maintenance branch.

@robertpanzer robertpanzer merged commit b466a81 into asciidoctor:main Sep 9, 2022
@robertpanzer robertpanzer mentioned this pull request Sep 10, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI should exit with non-zero status when e.g. missing include files
2 participants