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.

Sign up for GitHub

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

Jump to bottom

Move "org.asciidoctor.ast" package to "asciidoctorj-api" project #590

Closed

jmini wants to merge 5 commits into asciidoctor:asciidoctorj-1.6.0 from jmini:patch-1

Contributor

jmini commented Nov 10, 2017

convert classes to interfaces in api project
- Author
- ContentPart
- DocumentHeader
- RevisionInfo
- StructuredDocument
Create implementation in core project

jmini mentioned this pull request

Separate API from the JRuby implementation #589

Open

robertpanzer approved these changes

View reviewed changes

Member

robertpanzer left a comment

Looks good to me.
I also tested with the gradle and maven plugin and it seemed to work fine.

In the future I would like to have NodeConverter and NodeCache in an impl package as these depend on JRuby.
I'll wait a bit with the merge in case @mojavelinux or @lordofthejars have some comments.

asciidoctorj-api/build.gradle Outdated



		javadoc {
		classpath = sourceSets.main.output + sourceSets.main.compileClasspath + project(':asciidoctorj-test-support').sourceSets.test.output

Member

robertpanzer Nov 11, 2017

I think we don't need the asciidoctorj-test-support project for the java docs, right?

Contributor Author

jmini Nov 12, 2017

This is probabelly a copy paste error (I am not really a gradle user). What would be the correct value? nothing? Do I need a javadoc section?

Member

robertpanzer Nov 13, 2017

I guess this line can be completely removed. For this project we shouldn't need any particular classpath.

We would need this module though to create a complete javadoc for the API in asciidoctorj-api and asciidoctorj itself, but we can check that later.

jmini commented

View reviewed changes

asciidoctorj-api/src/main/java/org/asciidoctor/ast/ContentPart.java Outdated


		public List<ContentPart> getParts();

		public void setParts(List<ContentPart> parts);

Contributor Author

jmini Nov 13, 2017

I do not think that setParts(List<ContentPart>) should be on the API. The only usage is in JRubyAsciidoctor line 182 in JRubyAsciidoctor.getContentPartFromBlock(StructuralNode, int, int). At this point the setter on ContentPartImpl is enough.

This would be more consistent with the other method on this interface.

Member

robertpanzer Nov 13, 2017

👍

jmini force-pushed the patch-1 branch from 6f92c32 to 527acc7 Compare

November 13, 2017 19:13

Contributor Author

jmini commented Nov 13, 2017

Amended commit with the discussed modifications pushed...

robertpanzer reviewed

View reviewed changes

asciidoctorj-api/src/main/java/org/asciidoctor/ast/Author.java Outdated


		public String getInitials();

		public void setInitials(String initials);

Member

robertpanzer Nov 14, 2017

I think all these setters should go be on the impls only.
This API is only to extract structural information about the document, there's no way to modify these objects and have an impact on the underlying document.

jmini force-pushed the patch-1 branch from 527acc7 to 887de01 Compare

November 14, 2017 12:29

Contributor Author

jmini commented Nov 14, 2017

Setters removed...


          Move "org.asciidoctor.ast" package to "asciidoctorj-api" project

* convert classes to interfaces in api project
** Author
** ContentPart
** DocumentHeader
** RevisionInfo
** StructuredDocument
* Create implementation in core project

jmini commented

View reviewed changes

asciidoctorj-api/src/main/java/org/asciidoctor/ast/Author.java Outdated


		public interface Author {

		public String getFullName();

Contributor Author

jmini Nov 17, 2017

"public" is not necessary in interfaces

asciidoctorj-api/src/main/java/org/asciidoctor/ast/ContentPart.java Outdated


		public interface ContentPart {

		public String getId();

Contributor Author

jmini Nov 17, 2017

"public" is not necessary in interfaces

asciidoctorj-api/src/main/java/org/asciidoctor/ast/DocumentHeader.java Outdated


		public interface DocumentHeader {

		public List<Author> getAuthors();

Contributor Author

jmini Nov 17, 2017

"public" is not necessary in interfaces

asciidoctorj-api/src/main/java/org/asciidoctor/ast/RevisionInfo.java Outdated


		public interface RevisionInfo {

		public String getDate();

Contributor Author

jmini Nov 17, 2017

"public" is not necessary in interfaces

asciidoctorj-api/src/main/java/org/asciidoctor/ast/StructuredDocument.java Outdated

+               */
+              public interface StructuredDocument {
+                  public List<ContentPart> getParts();

Contributor Author

jmini Nov 17, 2017

"public" is not necessary in interfaces

jmini force-pushed the patch-1 branch from 887de01 to 4238359 Compare

November 17, 2017 07:38


          Remove public modifier in existing ast interfaces

7cdb7b9

jmini force-pushed the patch-1 branch 4 times, most recently from 6512c17 to 827d4b4 Compare

November 17, 2017 23:18

robertpanzer reviewed

View reviewed changes

asciidoctorj-api/src/main/java/org/asciidoctor/ast/Row.java Outdated

@@ @@ -4,6 +4,6 @@ @@
               public interface Row {
-                  public List<Cell> getCells();
+                  List<? extends Cell> getCells();

Member

robertpanzer Nov 18, 2017

I think this is not going to work.
While the getters on StructuredDocument etc are read-only collections, these are meant to be mutated by extensions.
With this change extensions will no longer be able to add columns, headers, footers, and rows.
I guess we have to figure out how to make the Groovy based tests spot such things.
As these tests are not compiled static at the moment Groovy just adds values to these members, but Java wouldn't allow it.

So using wildcards for these 5 interfaces should be fine.

Author
ContentPart
DocumentHeader
RevisionInfo
StructuredNode
Collections returned by all other AST interfaces should stay the same because they are intended to be mutated by extensions.

See for example

asciidoctorj/asciidoctorj-core/src/test/groovy/org/asciidoctor/extension/GithubContributorsBlockMacro.groovy

Line 52 in 5b80187

row.cells << avatarCell

Contributor Author

jmini Nov 20, 2017

I guess you mean StructuredDocument (StructuredNode does not exist)

Member

robertpanzer Nov 20, 2017

Of course, you're right.
This should've been StructuredDocument.


          Remove methods already defined in StructuralNode

dd9a6c0

Contributor Author

jmini commented Nov 19, 2017

I am not an expert in API design in Java, but here my ideas about it:

During my experimentations I have noticed that it would help me to have a relaxed definition of the generics parameter in List at API level. This is why I have proposed:

public interface Row {
    List<? extends Cell> getCells();
}

If you try to code a different implementation than the Ruby one (I use the prefix My in this example):
MyRow implements org.asciidoctor.ast.Row wants to return a list of MyCell. This implementation needs to control the implementation type of the getCells() list. Using List<Cell> in the implementation is wrong (for example it cannot accept org.asciidoctor.ast.impl.CellImpl or any other implementation that is out of control).

The implementation in MyRow is also something like:

public List<MyCell> getCells() {
    return this.cells;
}

Maybe my idea was naive, but I have noticed that at API level this means that I need the wildcard form in order to be able to compile everything.

For this pull request, I will revert the wildcard modification as you suggested in your review.

Let move forward on the point that have a consensus. (The need for wildcard everywhere is only requested if my experiment is a success and maybe the vision I have for an alternative plain Java implementation of the Asciidoctor AST is wrong anyway).

Member

robertpanzer commented Nov 19, 2017

The thing is that this shouldn't compile:

Row row = createTableRow();
Cell cell = createTableCell(column, "Some content");
List<? extends Cell> cells = row.getCells();
cells.add(cell)

The last statement shouldn't compile because cells could also be a List<SomeStrangeCell> if SomeStrangeCell implements Cell.
All the compiler knows about cell is that it implements the interface Cell, but not if it also extends SomeStrangeCell.

I see what you are aiming for and I agree that using plain lists also has its problems because if the user implements the AST interfaces himself without using our factory methods we fail as well.


          Replace deprecated method calls

330edc3

jmini force-pushed the patch-1 branch from 827d4b4 to 3e68d0c Compare

November 20, 2017 05:37

Contributor Author

jmini commented Nov 20, 2017

Thank you for your feedback.

I had understood your point, but now we have it explicitelly written in this PR. I think it is valuable to write down the arguments (pro and contra), in order to be able to start this discussion again in the future (if necessary).

I have pushed a new version with the discussed changes.

robertpanzer reviewed

View reviewed changes

asciidoctorj-core/src/test/java/org/asciidoctor/WhenAsciiDocIsRenderedToDocument.java Outdated

@@ @@ -298,20 +298,20 @@ public void should_get_attributes() { @@
                           "\n";
                       Document document = asciidoctor.load(documentWithAttributes, new HashMap<String, Object>());
-                      List<StructuralNode> blocks = document.getBlocks();
+                      List<? extends StructuralNode> blocks = document.getBlocks();

Member

robertpanzer Nov 20, 2017

Please don't do this.
I tend to show these tests to answer questions.
List<? extends StructuralNode> hides the possibility that this list can be mutated.
But we encourage that, see for example here:

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

Lines 31 to 39 in 5b80187

    
           List<StructuralNode> blocks = block.getBlocks(); 
        
           for (int i = 0; i < blocks.size(); i++) { 
        
               final StructuralNode currentBlock = blocks.get(i); 
        
               if(currentBlock instanceof StructuralNode) { 
        
                   if ("paragraph".equals(currentBlock.getContext())) { 
        
                       List<String> lines = ((Block) currentBlock).lines(); 
        
                       if (lines.size() > 0 && lines.get(0).startsWith("$")) { 
        
                           blocks.set(i, convertToTerminalListing((Block) currentBlock));

Contributor Author

jmini Nov 20, 2017

Of course. Sorry I missed that one by reverting...

Member

robertpanzer Nov 20, 2017

Ah ok, no worries.
I thought this was intentional :-)


          Add wildcard in the generics parameter

31587ba

jmini force-pushed the patch-1 branch from 3e68d0c to 31587ba Compare

November 20, 2017 13:30

Member

robertpanzer commented Nov 30, 2017

Thanks! 🎉

Member

robertpanzer commented Nov 30, 2017

Merged as fe43931

robertpanzer closed this

jmini mentioned this pull request

Remove wildcards in the generic type #605

Merged

Member

mojavelinux commented Oct 15, 2018

Nice work you two! 🎉

Member

mojavelinux commented Oct 15, 2018

I'd really like to see StructuredDocument / ContentPart removed from 1.6. That was a competing AST effort which, IMO, has caused a lot of confusion. Anything it does should be provided either from the official AST classes or through the main Asciidoctor interface. Let's start 1.6.0 with a clean AST package.

Member

robertpanzer commented Oct 15, 2018

Sounds good!

Member

mojavelinux commented Oct 15, 2018

Should I file an issue, or would you rather do it?

Member

robertpanzer commented Oct 15, 2018

It would be great if you could file an issue for it.
I would happily create a PR for it if nobody else wants to jumps in.

Member

mojavelinux commented Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment