-
Notifications
You must be signed in to change notification settings - Fork 173
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
Redirect Asciidoctor logging to Java Util Logging #643
Conversation
👍 I think this is an essential addition for 1.5.7, primarily for the build tools that use it. |
Changed the PR to first send notifications to observers of type @abelsromero Does this better meet your expectations? |
This works wonders! This really makes it really easy to handle the log from a build tool. And I can see the gradle plugin using Btw, I added a couple of silly comments to the code. |
@@ -84,68 +79,46 @@ public IRubyObject add(final ThreadContext threadContext, final IRubyObject[] ar | |||
progname = args[2].asJavaString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is marked as unused in my IDE
@@ -0,0 +1,8 @@ | |||
package org.asciidoctor.log; | |||
|
|||
import org.asciidoctor.ast.Cursor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import
mapSeverity(logRecord.getSeverity()), | ||
logRecord.getCursor() != null ? logRecord.getCursor().toString() + ": " + logRecord.getMessage() : logRecord.getMessage()); | ||
|
||
julLogRecord.setSourceClassName(logRecord.getSourceFileName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor is not captured here.
I fixed it locally adding julLogRecord.setParameters(new Object[] { logRecord.getCursor() });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this below.
(Just mentioning the obvious here so that I remember that I already handled this comment)
Thanks for the feedback! This is exactly what I needed! :) (And thanks a lot again!) |
It's still not 100% user friendly. But it's nice to have it if it can be done easily. We just need to document it clearly. |
@abelsromero Is this now in a usable state for you? |
There are some other conversations about possible features. Its fine for
me, but I'd wait a bit more
asciidoctor/asciidoctor-maven-plugin#261 (comment)
Wdyt?
…On Wed, May 9, 2018, 4:18 PM Robert Panzer ***@***.***> wrote:
@abelsromero <https://github.com/abelsromero> Is this now in a usable
state for you?
If yes I would merge it and then try to do a 1.5.7 release of AsciidoctorJ.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#643 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFg2oTyrC2WziYbtAOO5SYW0YMM1GFknks5twvqxgaJpZM4TzqGL>
.
|
Sure, np. |
@robertpanzer @abelsromero Truly great work on this you two! I'm impressed! |
Sorry about that. Sometimes file or dir is a URI. This is internal information that is leaking out. When I create the log records, I should probably |
I only see interesting the option to register via SPI. |
You mean to register an Observer automatically if it is declared in a file |
@mojavelinux Regarding asciidoctor/asciidoctor-gradle-plugin#233 (comment) Do you think it would make sense to always run with the verbose option when not running from the CLI? This way you would only configure the amount of logging in your logging framework. The CLI overrides |
Apologies for the silly question, but I don't get the purpose of verbose. From the command info |
I think the default log device is stderr: In my opinion it would make sense to always log these warnings independently of a verbose setting, at least when running embedded in the Maven or Gradle plugin. |
I am reading back my comment and I see it was horrible. What I meant was that I don't see any difference when using --verbose and not using it. |
Thanks for the clarification :) So would it make sense then to always enable this verbose flag to see those warnings? |
I'd vote for yes. On other topics, have you consider the option to register using SPI? (asking this I feel like when my manager comes in late Friday with a new requirement to implement) |
:) No worries! I considered adding an SPI and I already developed it (if I understood you correctly that it should be possible to register an Observer via the java.util.ServiceLoader mechanism). |
To go ahead I am planning to always set the When testing it I just stumbled over that the warning is logged without a context. |
Updated the PR to always set |
I second that! |
21d0ff5
to
a04f2cf
Compare
I'll merge for now. |
We've decided to go the other way on this. $VERBOSE is not something that should be set by default. The fact that it is linked to the invalid references warning is only because those warnings are not always accurate. In that regard, we should think of it more like "debug mode". But $VERBOSE does reveal programming errors, so it can be useful for other reasons. But certainly no something we want to enable by default. |
This is the version of #641 for the master branch.
In addition to only the changes from that PR I had to also pull in some more classes that didn't exist yet on master, i.e. Cursor/CursorImpl and RubyObjectWrapper.
I also changed the test to programmatically configure and register a Handler, the Maven plugin should be able to do just that as well.
Additionally I bumped the version of Java to 7 "officially" so that I don't have to downgrade some of the imported code. We're already relying on Java 7 now anyway.