Separate API from the JRuby implementation

[jmini]

For the moment, interfaces like the one defined in the org.asciidoctor.ast package for the AST are available in the asciidoctorj project. In my opinion, they should be separated in a asciidoctorj-api project, that have no dependency on JRuby.

---

In order to perform tests, there is the need to have a second project: asciidoctorj-tck that define tests on the object defined in asciidoctorj-api regardless of the underlying implementation.

I am not really familiar with this construct, but the tests in asciidoctorj-tck needs to be abstract and needs to delegate some functionality to the underlying implementation.
Take as example the test: WhenAsciiDocIsRenderedToDocument# should_find_elements_from_document()

The first line is implementation specific:

        Document document = asciidoctor.load(DOCUMENT, new HashMap<String, Object>());

This could correspond to an abstract method in the TCK:

        abstract String loadDOCUMENT();

(of course the DOCUMENT String should also be part of the TCK)

The rest of the method is not implementation specific and should be in the TCK:

        Map<Object, Object> selector = new HashMap<Object, Object>();
        selector.put("context", ":image");
        List<StructuralNode> findBy = document.findBy(selector);
        assertThat(findBy, hasSize(2));

        assertThat((String)findBy.get(0).getAttributes().get("target"), is("tiger.png"));
        assertThat(findBy.get(0).getLevel(), greaterThan(0));

I will start to experiment in this direction on the asciidoctorj-1.6.0 branch. If the result is good enough, I will propose a Pull-Request.

Please provide early feedback if you think the approach is wrong or if you have better patterns in minds to separate API and JRuby-implementation.

[mojavelinux]

This seems like a great direction for the 1.6.0 branch. 👍

[robertpanzer]

Sounds good.
If we split it up, should we already take modularization into account and try to make the packages disjoint?

[jmini]

I have started with the org.asciidoctor.ast package.

If we split it up, should we already take modularization into account and try to make the packages disjoint?

Any Idea what I should do with:

• org.asciidoctor.ast.NodeCache
• org.asciidoctor.ast.NodeConverter

They depend on JRuby and they should stay in the asciidoctorj-core project.
In my opinion package org.asciidoctor.ast belongs to asciidoctorj-api

I guess this is a split-package problem. Isn’t it?

[jmini]

I have started something here:
https://github.com/jmini/asciidoctorj/tree/patch-1

With commit 8742acd I have moved the org.asciidoctor.ast package to the asciidoctorj-api project.

[robertpanzer]

This is a nice start!
I added a comment to this commit.

NodeCache is a utility class that helps to keep the association between a Ruby node and the corresponding Java counterpart. It hides the Java node as a property of the Ruby node.
So this should stay in the impl module, same for the NodeConverter.
I cannot tell atm why it is in this package, moving it to an internal package shouldn't be so much of a problem.

The split package problem with Java 9 modules would require us to not share any packages between two jars, i.e. if org.asciidoctor.ast is in the api module, org.asciidoctor.ast.impl cannot be in another module.
(If we would ever reach this point, it would also require JRuby to work as a module, which looks like a lot of work, so this might be something to consider later, but possibly before a release of 1.6.0)

[jmini]

The split package problem with Java 9 modules would require us to not share any packages between two jars, i.e. if org.asciidoctor.ast is in the api module, org.asciidoctor.ast.impl cannot be in another module.

I was not aware that the packages are hierachical in Java 9.
Maybe we can use 'org.asciidoctor.ast_impl' in the asciidoctorj-core project. This way the package containing the implementation (currently org.asciidoctor.ast.impl) is not contained in org.asciidoctor.ast.

[robertpanzer]

I was not aware that the packages are hierachical in Java 9.

Just tested and you're right. Somehow I thought that if module A has package p1, the module B could not have p1.p2. But it seems like it seems to work indeed.

So simply forget my comment ;-)

[robertpanzer]

BTW it's probably best if you forget everything I was saying wrt Java 9.
If we can get closer to it now it would be nice, if not we'll get there later :-)

[jmini]

The packages also plays an important role for OSGi. I need to perform some tests in this area (since 2015 I stopped to use Equinox when I stop to work on Eclipse RCP Application), because if we let it like this org.asciidoctor.ast is now exposed by 2 jars.

---

About your remark on org.asciidoctor.internal.CaseInsensitiveMap it is used in the method: org.asciidoctor.ast.DocumentHeader.createDocumentHeader(Title, String, Map<String, Object>).

There are 4 classes in org.asciidoctor.ast that do not depend on JRuby but that are not interfaces:

• Author
• ContentPart
• RevisionInfo
• StructuredDocument

What is the idea here? Transform them to interfaces (this is probabelly more correct for an API project).

---

As user I have a good idea of the difference between an API project and one implementation, but I am really new at coding one.

---

I really like the approach of doing steps that are as small as possible (considering Java 9 modules later, just moving a first package, ...). I also would like to thank you for your advices and for your quick answers.

[robertpanzer]

These 4 classes seem to sit on top of the other AST classes.
I haven't touched them yet, as they don't come into play in extensions (to my knowledge)

But yes, it would be good to split out interfaces for these as well.

[jmini]

I have started a first Pull Request with the move of org.asciidoctor.ast to a new project: asciidoctorj-api.
#590

This a first step to solve this issue, but this is not the complete story.

My next step would be to create the testing support for the api project (input welcomed).

Then other packages should also be moved into the asciidoctorj-api project (for example org.asciidoctor.converter).

---

As far as I have tested, it is OK to have the same package in 2 OSGi modules.

That said, the two classes (org.asciidoctor.ast.NodeCache, org.asciidoctor.ast.NodeConverter) could be moved to org.asciidoctor.ast.impl of asciidoctorj-core as suggested before in this discussion.

[jmini]

While I was working on an alternative implementation, I noticed that it would be nice if the method signature for method returning list would be changed. The generics parameter should contains a wildcard. Example:

OLD:

public List<ContentPart> getParts();

NEW:

public List<? extends ContentPart> getParts();

See commit: c36f894

For me this is a second step/topic. What is your opinion, should I push something like that in PR #590 or should I wait?

[robertpanzer]

Makes sense to me as these lists are never passed back again as parameters to AsciidoctorJ.

[jmini]

Thank you for your feedback... What is your workflow for the multiple changes:

• One PR with multiple commits
• One PR with a single commit (for the subtopics)
• Multiple PR for the different subtopics

[robertpanzer]

Just do it in one PR, that's fine.
If you don't make it one commit I'll probably squash them when merging.
So feel free to rebase/fixup your PR as you like.
I'll test it out manually before merging.

(Sorry for being so slow in responding, days are long at the moment.)

[jmini]

I have pushed more commits to #590. The discussed change with wildcard and other cleanup commits that I made by reviewing the code.

(No worries about your review pace, for a non-commercial open-source project, it is good. I am very happy to get your feedback)

[jmini]

In order to write tests I need that are decoupled from the parser, I need to be able to instanciate a AST based on the ruby implementation from scratch.

See my question on the mailing list: API to create a valid AST from scratch

[robertpanzer]

I can't imagine that this is possible at the moment.
The NodeFactory isn't exposed as a public API and nodes are always bound to one Asciidoctor or Ruby instance.

[jmini]

new PR for the next package: #603 (org.asciidoctor.converter)

[jmini]

new PR for the org.asciidoctor package #606. org.asciidoctor.Asciidoctor can not be moved yet (A solution for extension is needed).