AsciidoctorMojo getAsciidoctorInstance updated to check the JRubyRunt…

[mattadamson]

…imeContext instance to determine whether either get or get(AsciiDoctor) overloads exist and invoke if the method exists. This ensures we can be compatible with both 1.5.x and 1.6.0 of asciidoctorj

[coveralls]

Coverage decreased (-1.7%) to 59.181% when pulling 7df5306 on mattadamson:master into 22fb1ab on asciidoctor:master.

[robertpanzer]

Thanks!

Without testing I have two remarks:

1. Could you please check the indentation of the code and adapt it to the style of the existing code? At least on GitHub the indentation seems a bit strange.
2. I think it is safe to catch all exceptions (except for the handled NoSuchMethodException) and rethrow the MojoException. Imo there is no need to explicitly catch every single declared exception.

Cheers
Robert

[mattadamson]

thanks @robertpanzer ,

1. Yes I was wondering whether we had an eclipse style formatter at all to auto format in this standard? If not where are the documented standards here e.g. tabs as space / indent level e.t.c.
2. I don't fully understand what you mean to change here. All the exceptions have to be caught as they are checked exceptions. The method only declares to throw MojoException so we have to catch all of them and rethrow

[robertpanzer]

Hi Matt,

There are no documented standards, just try to align with the rest of the
code, I guess it (and I personally prefer it ) l only uses spaces.

In 2. I meant to have sth like

catch (NoSuchMethodException e) { ...}
catch (Exception e) {throw new Mojo...(..., e);}

Does this pseudocode make sense to you?
In the inner block that handles the 1.6.0 case you can simple catch
(Exception) and rethrow it as a MojoException.

Having 5 or 6 catch clauses all doing the same doesn't seem to add any
value. And any other RuntimeException or other uncaught exception can also
result in a MavenException as long the original cause is not swallowed.
But that's surely only my personal opinion.

Cheers
Robert

Am Samstag, 7. Mai 2016 schrieb mattadamson :

thanks @robertpanzer https://github.com/robertpanzer ,

1. Yes I was wondering whether we had an eclipse style formatter at all to
   auto format in this standard? If not where are the documented standards
   here e.g. tabs as space / indent level e.t.c.
2. I don't fully understand what you mean to change here. All the
   exceptions have to be caught as they are checked exceptions. The method
   only declares to throw MojoException so we have to catch all of them and
   rethrow

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#218 (comment)

[mattadamson]

It's generally better not to catch the very general Exception type however I think we can improve this by using the ReflectiveOperationException type which is a base of most of the types. I also noticed some unchecked types were being caught when eclipse added the clauses. These can safely be removed

[coveralls]

Coverage decreased (-0.9%) to 60.057% when pulling 49553d7 on mattadamson:master into 22fb1ab on asciidoctor:master.

[robertpanzer]

That's nice, RuntimeExceptions can be removed, yes.

Is ReflectiveOperationException already in Java 1.6?
I'm currently unsure what Java versions are supported by the
asciidoctor-maven-plugin, but at least Asciidoctor 1.5.x is still running
on Java 1.6.

Cheers
Robert

Am Samstag, 7. Mai 2016 schrieb mattadamson :

It's generally better not to catch the very general Exception type however
I think we can improve this by using the ReflectiveOperationException type
which is a base of most of the types. I also noticed some unchecked types
were being caught when eclipse added the clauses. These can safely be
removed

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#218 (comment)

[robertpanzer]

Seems like compilation failed on Java 6 because ReflectiveOperationException is not available.

[mattadamson]

thanks it seems I had my JAVA_HOME set to 1.7 so didn't see this on full build. I've changed this to the generic Exception type for now however when we upgrade to 1.7+ I'd prefer we use a multiple type catch clause

[coveralls]

Coverage decreased (-0.6%) to 60.345% when pulling 0a82f65 on mattadamson:master into 22fb1ab on asciidoctor:master.

[mattadamson]

let me know if the current changes are ok and also what I need to do next to merge if happy.

[abelsromero]

I trust @robertpanzer with this, I can merge if he sees it perfect.
On my side, my only request is if you can squash all changes in a single commit. It helps trackng changes in the future.

thanks it seems I had my JAVA_HOME set to 1.7 so didn't see this on full build. I've changed this to the generic Exception type for now however when we upgrade to 1.7+ I'd prefer we use a multiple type catch clause

This happened to us all at some point or another. About the change to 1.7, feel free to open another issue to keep track of this.
It's the best way to avoid forgetting things.

[mattadamson]

thanks @abelsromero how can I squash all commits though if

1. They've been pushed already for review i.e. not just local commits
2. They've already been reviewed so would have to be reviewed in a new PR?

[robertpanzer]

Hi Matt,

I didn’t have the time yet to actually test it, I hope to make it today.
I will add a comment once I’m done (all my comments up to now have been just from looking at the code)

If it’s ok, you can simply squash them, across the Asciidoctor projects this is a common practice to squash multiple commits before merging.
No one will be wondering if a new fetch fails.

Cheers
Robert

Am 09.05.2016 um 09:07 schrieb mattadamson notifications@github.com:

thanks @abelsromero https://github.com/abelsromero how can I squash all commits though if

1. They've been pushed already for review i.e. not just local commits
2. They've already been reviewed so would have to be reviewed in a new PR?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #218 (comment)

[abelsromero]

1. They've been pushed already for review i.e. not just local commits

When you squash changes git will ask to override the changes using the "force" option, if you are using eclipse it's pretty easy and automatic, it has "squash" option. With IntellJ the process if more manual and tedious using "rebase". Little confesion...I keep an eclipse installation just for this ☺️

2. They've already been reviewed so would have to be reviewed in a new PR?

This PR will be updated with the new commit, the comments and kept, but the reference to the code is lost. But don't worry, this is a normal practice.

[robertpanzer]

Tested it and it seems to run fine.
I have one remark though:

AsciidoctorJ added support for configuring Extensions via annotations, so that for example block names do not have to be defined by the caller but are already encapsulated in the BlockProcessor, BlockMacroProcessor or InlineMacroProcessor.
But if I don't define a blockName externally, the Mojo will fail.

So should we extend https://github.com/asciidoctor/asciidoctor-maven-plugin/blob/master/src/main/java/org/asciidoctor/maven/extensions/AsciidoctorJExtensionRegistry.java#L57-L64 in a later PR or is it ok for now, @abelsromero
Without adapting this, new annotation based BlockProcessors, MacroProcessors and InlineMacroProcessors will not work.

To register these processors AsciidoctorJ added new methods AsciidoctorExtensionRegistry.block(), .blockMacro() and .inlineMacro() that just take the class without a name. (Therefore also only callable via reflection now.)

[abelsromero]

AsciidoctorJ added support for configuring Extensions via annotations

That's for 1.6 right? asciidoctor/asciidoctorj#343

If that's so, we can open an issue and label for 1.6.0. We already have some https://github.com/asciidoctor/asciidoctor-maven-plugin/milestones/1.6.0

[robertpanzer]

That's for 1.6 right? asciidoctor/asciidoctorj#343

Yes, exactly.

If that's so, we can open an issue and label for 1.6.0. We already have some https://github.com/asciidoctor/asciidoctor-maven-plugin/milestones/1.6.0

That would be nice, yes.

[robertpanzer]

Then I'm fine to merge.

[abelsromero]

@mattadamson if you need help with the squash, let us know. Or if it is easier, you can create a branch and send another PR

[mattadamson]

@abelsromero I'd like to learn how to do the squash after it's been pushed in this way. Do you have any links that will help?

[mojavelinux]

Keep in mind, it's now possible to squash from the Merge button. That saves a lot of headaches for contributors, IMO.

[mattadamson]

@mojavelinux really that sounds great? How do I do that? I'm used to using Stash at work and it would be nice if that also had a similar feature

[mattadamson]

Perhaps this is what you meant? https://help.github.com/articles/about-pull-request-merge-squashing/ I presume we use this version of github

[mojavelinux]

That's the one. It can only be done at the time of the merge, so it has to be done by the person merging the PR. As long as the Merge button is green (seen by any person with access), then the changes can be squashed to a single commit at the time the merge is performed.

[mattadamson]

@mojavelinux sure let's do that then with a suggestion the original commit message is used to reflect the entire change as only clean up since then

7df5306

[abelsromero]

I was not aware of the new squash feature, the squash option appears after pressing the merge button.

[mojavelinux]

Exactly. It surprised me the first time too. I've found it to be very useful. It's good when a PR contains multiple commits by a single person. When there are commits from different people, I tend to still use a manual rebase so that each person gets one commit.