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

[#737] Extension logging #755

Merged
merged 2 commits into from
Jan 26, 2019

Conversation

robertpanzer
Copy link
Member

This PR fixes #737 and allows extensions to also log LogRecords.

These LogRecords don't pass through Asciidoctor/Ruby though, so that handlers registered in Ruby won't get notified of them.

I had to make larger changes to the extension infrastructure to give extension access to their owning Asciidoctor instance.
@Mogztter This is a breaking change that will also hit you in AsciidoctorG, I renamed and changed the Interface that creates the extension delegates.
An extension now has a method unwrap(Class<?>) that allows to get the corresponding implementation.

@robertpanzer robertpanzer force-pushed the extension-logging branch 2 times, most recently from a2c2080 to 8717bce Compare January 12, 2019 19:42
@robertpanzer
Copy link
Member Author

@Mogztter Do you have any objections against these changes?

@ggrossetie
Copy link
Member

Sorry for the delay.

Do you have any objections against these changes?

No I do not have any objection 😉
So we now have two more methods in the API:

void log(LogRecord logRecord);
<T> T unwrap(Class<T> clazz);

The log method looks fine 👍
And we already talked about the unwrap method, it's necessary and useful.

I renamed and changed the Interface that creates the extension delegates.

I'm still trying to figure it out so I don't really mind for now.

@abelsromero
Copy link
Member

I haven't been on the whole conversation but should we add unwrap to the docs explaining clearly when it is required?

@robertpanzer
Copy link
Member Author

Good idea!

@robertpanzer
Copy link
Member Author

@Mogztter Awesome, thanks! I wanted to have your approval as this will break your current AsciidoctorG.

I was thinking about creating a asciidoctorj-1.6.0 branch soon, and then let master evolve to 2.0.0.
So as this changes the interface of a Processor I don't want to merge it into 1.6.

@ggrossetie
Copy link
Member

I was thinking about creating a asciidoctorj-1.6.0 branch soon, and then let master evolve to 2.0.0.
So as this changes the interface of a Processor I don't want to merge it into 1.6.

Why not, asciidoctorj-1.6.0 will be a "maintenance" branch for AsciidoctorJ 1.6.x ?
As far as I know, Asciidoctor 2.0.0 (core) will be released soon (end of January) so indeed we should merge it into master to prepare the ground for 2.0.0.

@mojavelinux
Copy link
Member

I think the branch should be named asciidoctorj-1.6.x (or asciidoctorj-1.6.0.x). Aside from that, 👍

Once 2.0.0 is out, I'd like to use a more standard branch naming pattern. I've been using v2.0.x in other projects that use SemVer, though I'm open to discuss the "v" prefix. Perhaps best in a separate issue.

@ggrossetie
Copy link
Member

Once 2.0.0 is out, I'd like to use a more standard branch naming pattern. I've been using v2.0.x in other projects that use SemVer, though I'm open to discuss the "v" prefix. Perhaps best in a separate issue.

Since npm creates tags prefixed by v, I don't use this prefix in branches names for usability reasons.
So when I type git checkout v (+ tab for expansion) I know that git will only show me tags. And when I type git checkout 1 I will only get branches (in this case maintenance branches for 1.x).

I know that Maven prepends the artifactId but I don't think it's necessary and it can be overridden:

<tagNameFormat>v@{project.version}</tagNameFormat>

http://maven.apache.org/maven-release/maven-release-plugin/examples/prepare-release.html

It should be possible to do the same with Gradle. Not saying that we should do it but it's possible to do so if we want to adopt a new format 😉

@robertpanzer
Copy link
Member Author

I don't have any specific preference :-)
There's so many different ways each having its own advantages and disadvantages, that I am sure that whatever we choose, any possible disadvantage will bite us at some time 😅.

asciidoctorj-1.6.x would continue the tradition of asciidoctorj-1.5.x.

@robertpanzer
Copy link
Member Author

Rebased.
This PR breaks binary compatibility with 1.6.x, there fore it must not be merged into the 1.6.x branch.

@mojavelinux
Copy link
Member

So when I type git checkout v (+ tab for expansion) I know that git will only show me tags.

That's a strong argument for. 🤔

@robertpanzer
Copy link
Member Author

I already went ahead and created a branch asciidoctorj-1.6.x.
I can certainly still rename it, but it would match the pattern of not starting with a v. :)

@mojavelinux
Copy link
Member

I don't think any branch renames are necessary until 2.0.0. The 2.0.0 is the dawn of the new era where we try new things.

@robertpanzer robertpanzer merged commit 40aa34f into asciidoctor:master Jan 26, 2019
@robertpanzer robertpanzer deleted the extension-logging branch January 26, 2019 10:35
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.

Use the Asciidoctor logger from a Java Extension
4 participants