Concordion changes that could benefit the Excel Extension

[Nigel Charman]

Rob - you mentioned on the Concordion list about potentially reusing the concise expression language from the Markdown parser fir the Excel extension. I've now pulled this out into a separate ConciseExpressionParser class in Concordion core. See ConciseExpressionParserTest for usage examples. Would you review this for use with your extension and provide feedback please.

I've also introduced a new withSpecificationType(String typeSuffix, SpecificationConverter specificationConverter) method into ConcordionExtender. As well as making it easier to add new specification types to Concordion, it fixes a number of issues with the current mechanism around allowing multiple specification type suffices, breadcrumb generation, linking to other specs with the suffix with concordion:run etc.

The SpecificationConverter interface is simple to use:

public interface SpecificationConverter {

    InputStream convert(Resource resource, InputStream inputStream) throws IOException;
}

This replaces the need to have separate SpecificationLocator, Source and Target implementations, and simplified the markdown support from needing:

withSpecificationLocator(locator).withSource(source).withTarget(target);

to:

withSpecificationType("md", markdownConverter);

This will be published in the upcoming 2.0-SNAPSHOT and released with Concordion 2.0. Would you update the ExcelExtension to use this new method please, and we can publish an updated ExcelExtension alongside the Concordion 2.0 release?

cheers
Nigel

[bobmkite9]

Hi Nigel,

I’ve blocked out some time on Friday to take a look at it.  

cheers,

Rob

[Nigel Charman]

Great thanks.

Note that these changes have not been built into a SNAPSHOT version yet, but they have been merged to master. I've added a section to the README on publishing to Maven local, so you can try out the changes locally.

[bobmkite9]

ok, just a couple of things:

1.  Shouldn’t the Markdown stuff be in an extension, rather than in the concordion package?  It’s adding extra dependencies to the main concordion project which seems a bit wrong.  

2.  It looks like ConciseExpressionParser is converting back into xml attributes, which are then handled by the DocumentParser.  Wouldn’t it make more sense to factor this out so that we have some interface for generating the CommandCall tree like:

interface CommandCallTreeBuilder {

public CommandCall generateCommandCallTree(Element x, Resource r)

}

Then the ConciseExpressionParser could implement this, DocumentParser could implement this and we could have a method in ConcordionExtender to set up which approach you want to use.  And this could live in the main concordion project, to be used by both markdown, excel etc.

3.  withSpecificationType makes perfect sense.  I’ll update this too.

[Nigel Charman]

comments inline:

On 16/01/16 00:43, bobmkite9 wrote:

ok, just a couple of things:

1.  Shouldn’t the Markdown stuff be in an extension, rather than in the concordion package?  It’s adding extra dependencies to the main concordion project which seems a bit wrong. 

The feedback from the extension indicated that the Markdown format would see widespread adoption. To this end, moving it into core makes it as easy as possible for users to adopt. We're working on a website 2.0 and changing the focus of the tutorials etc to be around the Markdown format, with the HTML format also documented as an option.

The extra dependencies add about 1Mb to the distribution. Is this a serious concern to anyone?

2.  It looks like ConciseExpressionParser is converting back into xml attributes, which are then handled by the DocumentParser.  Wouldn’t it make more sense to factor this out so that we have some interface for generating the CommandCall tree like:

interface CommandCallTreeBuilder {

public CommandCall generateCommandCallTree(Element x, Resource r)

}

Then the ConciseExpressionParser could implement this, DocumentParser could implement this and we could have a method in ConcordionExtender to set up which approach you want to use.  And this could live in the main concordion project, to be used by both markdown, excel etc.

So that the "converters" create an XOM element tree directly rather than an intermediate HTML format? We did consider this while developing the Markdown support, but decided to leave as-is for now, since we didn't want XOM to be the canonical format. Ideally we'd like to reduce our dependency on XOM, and use a different format for the element tree. We do already have quite a bit of leakage, eg. Document being passed to DocumentParsingListener that extensions are using, so this would be a major change. As an interim step, if we could wrap XOM behind another API for generating the command call tree, it would be more palatable. I wouldn't rule this out for the future. If anyone is keen to work on this, please reply to the list :)

We would need to create a Document rather than the Element tree too, since Concordion relies on implementations of the DocumentParsingListener for creating some aspects of the target spec. This wouldn't be a biggie since we could have a default Document that is used if not supplied.

3.  withSpecificationType makes perfect sense.  I’ll update this too.

thanks :)

[Nigel Charman]

FYI - this interface changed before the release of the latest snapshot. The method's signature is now

InputStream convert(InputStream inputStream) throws IOException;

As well as simplifying the interface, it also ensures that the Source is used to read the Resource.