Add missing method implementations to Java AST classes and fixed some…

[robertpanzer]

… misalignments.

As a continuation of #498 this adds more methods missing in the Java AST classes that are required if nodes are touched once by a Treeprocessor.

Additionally all occurrences of .context are now replaced with .context.to_sym.
The contexts of content created by Blockmacros could for example never be handled correctly by the parser.

This PR should also fix the second issue mentioned in #497 .

[mojavelinux]

It might be a lot cleaner to simply monkeypatch the AbstractNode (in src/main/resources/org/asciidoctor/internal/asciidoctorclass.rb) to override the context getter method.

module Asciidoctor
    class AbstractNode
        def context
            @context.to_sym
        end
    end
end

That still won't fix internal references to @context, but it at least addresses all the public uses.

I really don't feel good about modifying the Ruby source code from the build. This eliminates that need.

[robertpanzer]

Thanks for the tip!
I'll try that out as soon as I'm sitting in front of a big screen again.
Nevertheless I'd intuitively say that it won't be called for Java nodes as
they don't really care about this class (my Ruby knowledge is very poor
though)

I don't feel good about monkeypatching either.
At the same time I am wondering why we don't have a lot more issues when
working with Extensions.
Many comparisons simply seem to return false always as soon as
Treeprocessors, Blockmacros or Block extensions come into play.

Am Samstag, 13. August 2016 schrieb Dan Allen :

It might be a lot cleaner to simply monkeypatch the AbstractNode (in
src/main/resources/org/asciidoctor/internal/asciidoctorclass.rb) to
override the context getter method.

module Asciidoctor
class AbstractNode
def context
@context.to_sym
end
endend

That still won't fix internal references to @context, but it at least
addresses all the public uses.

I really don't feel good about modifying the Ruby source code from the
build. This eliminates that need.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#499 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABHBjlYenO4yvlQsIHzGZpkwYj-vvwjpks5qfjtagaJpZM4JjuwV
.

[mojavelinux]

I don't feel good about monkeypatching either.

I don't mind monkeypatching. I just don't like that we are modifying the code in core directly instead of extending/overridding the Ruby code.

Nevertheless I'd intuitively say that it won't be called for Java nodes as
they don't really care about this class

This is probably my misunderstanding of how 1.5.x works, but as far as Ruby is concerned, the underlying context method will have changed. As long as the Java is hitting the context method to get the value, this will work.

Many comparisons simply seem to return false always as soon as
Treeprocessors, Blockmacros or Block extensions come into play.

That's why I advocate so strongly for anyone using extensions to be using AsciidoctorJ 1.6.x. I don't think we want to spend a lot of time trying to fix 1.5.x when we know that the model is wrong. We should only address the high priority issues and keep encouraging people to switch.

[robertpanzer]

The thing with the Java nodes is that they sit directly between Asciidoctor
Ruby and the original Ruby node, aka the "delegate", as soon as they are
touched once by Java.
These Java nodes have nothing in common with the Ruby classes except
methods that are magically duck typed against the called methods, so every
call of context that is on a Java node will return a string.

That's the major difference between Asciidoctorj 1.5 and 1.6: in 1.6 this
is no longer the case: Everything that is in Ruby stays in Ruby, the Ruby
world should never see Java implementations.

Am Sonntag, 14. August 2016 schrieb Dan Allen :

I don't feel good about monkeypatching either.

I don't mind monkeypatching. I just don't like that we are modifying the
code in core directly instead of extending/overridding the Ruby code.

Nevertheless I'd intuitively say that it won't be called for Java nodes as
they don't really care about this class

This is probably my misunderstanding of how 1.5.x works, but as far as
Ruby is concerned, the underlying context method will have changed. As
long as the Java is hitting the context method to get the value, this
will work.

Many comparisons simply seem to return false always as soon as
Treeprocessors, Blockmacros or Block extensions come into play.

That's why I advocate so strongly for anyone using extensions to be using
AsciidoctorJ 1.6.x. I don't think we want to spend a lot of time trying to
fix 1.5.x when we know that the model is wrong. We should only address the
high priority issues and keep encouraging people to switch.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#499 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABHBjs4xkeHJttj4PabqClTfclz4XkSKks5qfj_2gaJpZM4JjuwV
.

[mojavelinux]

in 1.6 this is no longer the case: Everything that is in Ruby stays in Ruby, the Ruby world should never see Java implementations.

That means the patch I proposed may not work. If that's the case, then we'll have to use the solution in the PR.

[robertpanzer]

Just tried adding the script adding some debug outputs without luck

As soon as a Treeprocessor touches a node, the Java version of AbstractNodeImpl.getContext() is called instead of the Ruby AbstractNode#context.
So I fear that as long as we don’t find another way to fix this, monkeypatching all calls of context to context.to_sym is the only way to make AsciidoctorJ behave the same way as Asciidoctor.

Just to make it explicit:
The additional global monkeypatching is NOT necessary to fix the exception Alexander reported in #497!
It was more that this additional example made me think again about how the current integration works, and that we have differences here.
To fix this error it was sufficient to implement the additional methods in the Java objects. (Which is also a sign that the original Ruby classes are completely out of the game after the conversion)
If we feel that we can live with these differences it would be sufficient to only add the missing Java methods and reduce the patching again.
We already live so long with these differences, maybe we can continue living with them until 1.6 when these problems should be fixed conceptually. (Waiting for errors to come:-) )

Cheers
Robert

Am 14.08.2016 um 00:21 schrieb Dan Allen [email protected]:

in 1.6 this is no longer the case: Everything that is in Ruby stays in Ruby, the Ruby world should never see Java implementations.

That means the patch I proposed may not work. If that's the case, then we'll have to use the solution in the PR.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #499 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABHBjuJS-9BSnRPSrPZbovj-1NQVcA5Tks5qfkN9gaJpZM4JjuwV.

[mojavelinux]

(Which is also a sign that the original Ruby classes are completely out of the game after the conversion)

That's the clarification I was looking for. Thank you.

We already live so long with these differences, maybe we can continue living with them until 1.6 when these problems should be fixed conceptually. (Waiting for errors to come:-) )

That's the strategy I support. I think we need to be looking ahead to 1.6 instead of backwards at 1.5. That does put some more pressure on the timeline, but I think that's a good thing.

[robertpanzer]

OK, then I’ll go back to monkeypatching only the comparison in abstract_node.rb that we currently have on master.

Am 14.08.2016 um 22:34 schrieb Dan Allen [email protected]:

(Which is also a sign that the original Ruby classes are completely out of the game after the conversion)

That's the clarification I was looking for. Thank you.

We already live so long with these differences, maybe we can continue living with them until 1.6 when these problems should be fixed conceptually. (Waiting for errors to come:-) )

That's the strategy I support. I think we need to be looking ahead to 1.6 instead of backwards at 1.5. That does put some more pressure on the timeline, but I think that's a good thing.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #499 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABHBjuH2-IkGX62I_y6Rpuyuvw12x6Rlks5qf3vZgaJpZM4JjuwV.

[mojavelinux]

Cool. Technically, that's not monkey patching. That's hot patching. Monkey patching refers to loading a Ruby file that modifies a type already defined (such as replacing a method).

[robertpanzer]

Technically, that's not monkey patching. That's hot patching. Monkey patching refers to loading a Ruby file that modifies a type already defined (such as replacing a method).

Thanks for the clarification! I didn't know that difference yet.
Heard the term monkey patching in Java in the past when classes with the same name as a class in an imported library were defined to override behavior.

If you're ok with the current state of this PR I would merge it.

[mojavelinux]

👍