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

Is the isRole call properly propogated? #323

Closed
ysb33r opened this issue Jun 1, 2015 · 10 comments
Closed

Is the isRole call properly propogated? #323

ysb33r opened this issue Jun 1, 2015 · 10 comments
Assignees
Labels
Milestone

Comments

@ysb33r
Copy link
Member

ysb33r commented Jun 1, 2015

This follows on from #316.

Given this document,

= Working with Images

[chapter]
== Block image

=== Block image with role

.Image One Left
[.left.text-center]
image::Image1.png[]

When the transform is image, try to print the node.role

A partial exception is

        Caused by:
        org.jruby.exceptions.RaiseException: (TypeError) cannot convert instance of class org.jruby.RubyString to boolean
            at org.jruby.gen.InterfaceImpl1017458621.isRole(org/jruby/gen/InterfaceImpl1017458621.gen:13)
            at org.ysb33r.asciidoctor.leanpub.LeanpubConverter.convertImage(LeanpubConverter.groovy:353)
            at org.ysb33r.asciidoctor.AbstractTextConverter.convert(AbstractTextConverter.groovy:102)
            at org.asciidoctor.converter.ConverterProxy.convert(org/asciidoctor/converter/ConverterProxy.java:90)
            at RUBY.convert(/Users/schalkc/.m2/repository/org/asciidoctor/asciidoctorj/1.6.0-SNAPSHOT/asciidoctorj-1.6.0-SNAPSHOT.jar!/gems/asciidoctor-1.5.2/lib/asciidoctor/abstract_block.rb:71)
            at RUBY.content(/Users/scha

BTW it does not fail for the normal converters (which are all in Ruby).

@robertpanzer
Copy link
Member

Copied my comment from #316:

It seems that AbstractNode.isRole() doesn't delegate to AbstractNode::role? but to the property reader AbstractNode::role.
I guess that if it returns nil it is converted to false but it can't do that if the role is set.

Not sure how to deal with that.
Copy the logic of AbstractNode::role?into AbstractNode.isRole()?
Or use Ruby reflection powers to explicitely invoke role? on the Ruby object instead of relying on JRubys Ruby/Java mapping magic.

@robertpanzer
Copy link
Member

I validated this behavior by adding some putss into role? and role.

@robertpanzer
Copy link
Member

@mojavelinux If we do not trust JRuby method invocation magic we could try to make every call explicit and go into the direction we took with the converter and extension proxies, i.e. use Helpers.invoke() to explicitely methods on the Ruby objects.

@robertpanzer
Copy link
Member

@ysb33r To work around your problem:
You surely want to get the role instead of asking if a role is set.
Therefore you should directly invoke getRole().
This is another inconvenience now in Groovy ;-) node.role will invoke node.isRole() instead of node.getRole().
And if you really want to call node.isRole() you can invoke node.getRole() != null instead.

@ysb33r
Copy link
Member Author

ysb33r commented Jun 1, 2015

This is another inconvenience now in Groovy ;-) node.role will invoke node.isRole() instead of node.getRole().

Well, well, well, interesting behaviour, for if I use the following Groovy code, getRole() is called for Groovy 2.4.x, but isRole() is called for Groovy 2.3.x.

class A {
  boolean isRole() {true}
  String getRole() {'role'}
}

def a = new A()
println a.role

@robertpanzer
Copy link
Member

😭 Wow, that change can really break code and it's very hard to spot the error.

@ysb33r
Copy link
Member Author

ysb33r commented Jun 1, 2015

I started a question regarding this behaviour on the Groovy mailing list - http://groovy.markmail.org/search/?q=#query:+page:1+mid:eb6b3pt5v72v2qxb+state:results

@robertpanzer
Copy link
Member

I think I prefer getting rid of JRuby method invocation magic and doing it all explicitely.
That means that the nodes delegates should no longer be AbstractNodes but IRubyObjects.
Then we are clear about what object belongs to what runtime and we don't get tempted to invoke sth that we don't really know.

@mojavelinux
Copy link
Member

On top of that change, I think we should get rid of the isRole() method. Instead, it should just be hasRole() with no arguments. The accessor here is really getRole() and we want that to win out.

@robertpanzer
Copy link
Member

Fixed with #317
Changed method isRole() to hasRole()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants