Fixes and Extensions to the IncludeProcessor and Reader classes #537

mmews-n4 · 2016-12-07T12:12:21Z

This PR includes the following three changes:

Fix of IncludeProcessor#handles method

The handles(String) method in the IncludeProcessor is not called, because the annotation on the IncludeProcessorProxy#handles method lacks a question mark. Test case added.

Extended IncludeProcessor to accept positional attributes

Before, the IncludeProcessor ignored any positional attributes. Now they can be used separated by commas. Internally, they are key value pairs where the keys are numbered accordingly to their order in the squared macros. The fix is only that these key value pairs are also evaluated when constructing the attributes map. Test case added.

Extended Reader and ReaderImpl

The reader now provides the methods getFile and getDir which are delegated to 'file' and 'dir' of asciidoctor.

…doctorj-1.6.0

…ngs.

…doctorj-1.6.0

… not have an inner document.

…tributes of a block that was previously created by a block processor.

…doctorj-1.6.0

- Fixed handles method in IncludeProcessor. - Added methods to ReaderImpl. - Added anonymous attributes to IncludeProcessor.

- Fixed handles method in IncludeProcessor. - Added anonymous attributes to IncludeProcessor.

robertpanzer · 2016-12-07T14:50:10Z

Awesome, thank you!
Great that you discovered that the handles() method wasn't actually called for IncludeProcessors.

I am personally ok with mapping the positional args to the strings "1", "2", ...

Could you please check why the tests are failing on Travis with Java 7?

Another – maybe pedantic – note: Could you please check the indentation of the source code? Looks like you're using Tabs instead of Spaces which makes the code look a bit weird on Github.

mojavelinux · 2016-12-07T21:54:57Z

I am personally ok with mapping the positional args to the strings "1", "2", ...

I believe that will solve some other type issues we were having. I was using numbers here to avoid collision with attributes that have the same name, but we could just say that numeric keys (1=value) are reserved for another purpose and should not be declared explicitly.

Extended IncludeProcessor to accept anonymous attributes

Do you mean positional attributes (value only)? If so, could you update the terminology? (See http://asciidoctor.org/docs/user-manual/#setting-attributes-on-an-element)

If I try using an IncludeProcessor written in Ruby, I get these attributes:

include::other.adoc[foo,bar,a=b]

gives me:

{1=>"foo", 2=>"bar", "a"=>"b"}

So effectively this fix translates those numeric keys into string keys, correct?

mojavelinux · 2016-12-07T21:55:23Z

Btw, thanks for this fix!

mojavelinux · 2016-12-08T07:37:17Z

👍

mmews-n4 · 2016-12-08T07:37:37Z

Thanks for your feedback!

I replaced the tabs by spaces, as you preferred, @robertpanzer.
@mojavelinux, you're right, this fix translates those numeric keys into string keys.
I also replaced the term 'anonymous' by 'positional' to fit in to the terminology.
I tried to reproduce the errors in the Java7 environment, but it seems to work with JDK7 on my machine. Do you have some advice on how I could reproduce the failure?

robertpanzer · 2016-12-10T05:26:35Z

Sorry for responding so late!

I just tried out the failing test.
I guess this is due to the extension iterating over the attributes Map and concatenating the results.
The map is not ordered in any way and the order of the elements given by a HashMap definitely differs between Java 7 and 8. (Stumbled myself over this change a couple of times)
So for me the test fails with this error:

java.lang.AssertionError: 
Expected: a string starting with "My,Positional,Attribute List"
     but: was "Attribute List,Positional,My"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.junit.Assert.assertThat(Assert.java:956)
	at org.junit.Assert.assertThat(Assert.java:923)
	at org.asciidoctor.extension.WhenJavaExtensionIsRegistered.a_include_processor_can_handle_positional_attrs(WhenJavaExtensionIsRegistered.java:430)

You might simply want to either sort the entries by key ("1", "2" and "3") or just test if the expected substrings are present.

robertpanzer · 2016-12-10T05:34:34Z

asciidoctorj-core/src/main/java/org/asciidoctor/extension/Reader.java

+	 *
+	 * @return file name
+	 */
+	public String getFile();


Also, could you please change these tabs to spaces?
(Sorry, but I am pretty opinionated on this 😁

robertpanzer · 2016-12-10T05:35:11Z

asciidoctorj-core/src/main/java/org/asciidoctor/extension/ReaderImpl.java

-    @Override
+	public String getFile() {
+		IRubyObject rObj = getRubyProperty("file");
+		return rObj.toString();


robertpanzer · 2016-12-10T05:35:47Z

asciidoctorj-core/src/test/java/org/asciidoctor/extension/WhenJavaExtensionIsRegistered.java

@@ -395,6 +396,42 @@ public void a_include_instance_processor_should_be_executed_when_include_macro_i
    }

    @Test
+	public void a_include_processor_should_only_handle_its_handles() {


Here as well, tabs.

mmews-n4 · 2016-12-12T07:21:35Z

@robertpanzer thanks a lot for your advice! Hopefully the tests will be green now!

mmews-n4 · 2016-12-12T09:20:11Z

@robertpanzer, I don't know the cause of the error. The message says, I should look at
file:///C:/projects/asciidoctorj/asciidoctorj-pdf/build/reports/tests/index.html (which I cannot access) to get information about the error. Do you know the cause of the error?

robertpanzer · 2016-12-14T06:58:23Z

Awesome!
Thank you for this great contribution!

abelsromero · 2016-12-14T07:50:18Z

Do you know the cause of the error?

@mmews-n4 The full report is not accessible now, but the output shows this SEVERE: (RuntimeError) asciidoctor: FAILED: required gem 'rouge' is not installed. Processing aborted.

Highly likely, to be related to this: https://github.com/robertpanzer/asciidoctorj-pdf/issues/3

kduske-n4 and others added 15 commits September 5, 2016 15:39

Ignore bin/ directory produced by Eclipse.

322b70e

Recursively convert nested maps to ruby hashes.

b888ad0

Merge remote-tracking branch 'upstream/asciidoctorj-1.6.0' into ascii…

da27101

…doctorj-1.6.0

Convert nested Java maps to Ruby hashes by assuming the keys are stri…

5d5885a

…ngs.

Return null if the inner document of a table cell is nil.

4179176

Add getter for @content_model variable of AbstractBlock.

910d62d

Merge remote-tracking branch 'upstream/asciidoctorj-1.6.0' into ascii…

fdec5df

…doctorj-1.6.0

Add test for content_model getter on StructuralNode.

36a6608

Add test case for accessing the inner document of table cells that do…

aa97f3d

… not have an inner document.

Add test case for avoiding a ClassCastException when accessing the at…

e1bd9ed

…tributes of a block that was previously created by a block processor.

Formatting.

ec92620

Fix a few code style problems.

f29d27a

Merge remote-tracking branch 'upstream/asciidoctorj-1.6.0' into ascii…

6cd79e9

…doctorj-1.6.0

Three fixes/extensions:

50a19f7

- Fixed handles method in IncludeProcessor. - Added methods to ReaderImpl. - Added anonymous attributes to IncludeProcessor.

Extended Reader to match ReaderImpl. Added tests for fixes:

4f57aab

- Fixed handles method in IncludeProcessor. - Added anonymous attributes to IncludeProcessor.

Same changes to fix the java7 error

2d5bd7f

Replaced 'Anonymous' by 'Positional'. Replaced tabs by spaces.

2c79fb3

robertpanzer reviewed Dec 10, 2016

View reviewed changes

Changed test to sort entries first, replaced more tabs by spaces

ec0e031

robertpanzer merged commit 6544bf2 into asciidoctor:asciidoctorj-1.6.0 Dec 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and Extensions to the IncludeProcessor and Reader classes #537

Fixes and Extensions to the IncludeProcessor and Reader classes #537

mmews-n4 commented Dec 7, 2016 •

edited

Loading

robertpanzer commented Dec 7, 2016

mojavelinux commented Dec 7, 2016

mojavelinux commented Dec 7, 2016

mojavelinux commented Dec 8, 2016

mmews-n4 commented Dec 8, 2016

robertpanzer commented Dec 10, 2016

robertpanzer Dec 10, 2016

robertpanzer Dec 10, 2016

robertpanzer Dec 10, 2016

mmews-n4 commented Dec 12, 2016

mmews-n4 commented Dec 12, 2016 •

edited

Loading

robertpanzer commented Dec 14, 2016

abelsromero commented Dec 14, 2016

Fixes and Extensions to the IncludeProcessor and Reader classes #537

Fixes and Extensions to the IncludeProcessor and Reader classes #537

Conversation

mmews-n4 commented Dec 7, 2016 • edited Loading

Fix of IncludeProcessor#handles method

Extended IncludeProcessor to accept positional attributes

Extended Reader and ReaderImpl

robertpanzer commented Dec 7, 2016

mojavelinux commented Dec 7, 2016

mojavelinux commented Dec 7, 2016

mojavelinux commented Dec 8, 2016

mmews-n4 commented Dec 8, 2016

robertpanzer commented Dec 10, 2016

robertpanzer Dec 10, 2016

Choose a reason for hiding this comment

robertpanzer Dec 10, 2016

Choose a reason for hiding this comment

robertpanzer Dec 10, 2016

Choose a reason for hiding this comment

mmews-n4 commented Dec 12, 2016

mmews-n4 commented Dec 12, 2016 • edited Loading

robertpanzer commented Dec 14, 2016

abelsromero commented Dec 14, 2016

mmews-n4 commented Dec 7, 2016 •

edited

Loading

mmews-n4 commented Dec 12, 2016 •

edited

Loading