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

Multiple small changes #509

Merged

Conversation

kduske-n4
Copy link
Contributor

Sorry to submit multiple changes in one pull request (and not using branches), but I had to make these changes in quick succession and could not isolate them. They are independent of each other, however. There are three changes:

Convert nested Java maps to Ruby hashes in RubyHashUtil#toRubyObject
This prevents a ClassCastException when, after creating a new block in a block processor, you attempt to access the attributes of that block later in a tree processor. When asking the block for its attributes, AsciidoctorJ assumes that what it gets from the asciidoctor AST is an instance of RubyHash, when in this case what it really gets is an instance of MapJavaProxy that was injected by JRuby into the asciidoctor AST when the block was created originally.

I'm not sure about this change, since I had to choose one of several different methods to convert Java maps to Ruby hashes in RubyHashUtil.

Return null if the inner document of a table cell is nil
This prevents a NullPointerException when attempting to access the inner document of a table cell in a processor when that cell doesn't have an inner document in the asciidoctor AST.

Add getter for @content_model
This adds a getter for the @content_model attribute of an asciidoctor AbstractBlock to StructuralNode.

I hope these changes are useful for you and that you can apply them. I have run the tests in the asciidoctorj-asciidoctorj and asciidoctorj-distribution projects and there were no failures.

IRubyObject innerDocument = getRubyProperty("inner_document");
if (innerDocument.isNil())
return null;
return (Document) NodeConverter.createASTNode(innerDocument);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it, but I don't think this will compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, though. Why shouldn't it?

Copy link
Member

@robertpanzer robertpanzer Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! :-)
Could you please add braces around the return null?

I probably automatically put them in there mentally around both returns that have the same indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@robertpanzer
Copy link
Member

Thanks a lot for your great contribution, Kristian!

Would you mind adding tests for the three problems you fixed?
That should be

That would help us a lot and would avoid that we later stumble over these problems again.

@kduske-n4
Copy link
Contributor Author

Added test cases as requested.

@robertpanzer
Copy link
Member

Thanks a lot! Such tests really help us.
Executed them locally and they look good, if I revert the main code, I can also see the errors that you fixed.

There are a couple of code style problems that codenarc complains.
Would you mind fixing them?
While it might seem pedantic it helps us to keep a common code style across the groovy code.
When doing a ./gradlew clean build codenarc will create a report that shows you what it doesn't like.

Thanks a lot!

Cheers
Robert

@kduske-n4
Copy link
Contributor Author

Sure, will do!

@kduske-n4
Copy link
Contributor Author

Done ;-)

@kduske-n4
Copy link
Contributor Author

Some things still seem broken in your CI. I have run the tests locally and it was fine. Any hints?

@robertpanzer robertpanzer merged commit 543ad9d into asciidoctor:asciidoctorj-1.6.0 Sep 9, 2016
@robertpanzer
Copy link
Member

Yes, there are some problems with Asciidoctor PDF and JRuby on Windows.
That's why this test is in. :-)
Will upgrade the JRuby version soon and look if it works better now (even though I don't have much hope)
Unix build is green, so I'm fine to merge.

Thanks!

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.

2 participants