[samukce]

This PR is one idea to make an extensible inline process through the interface NodeExtension where the client can implement how they want that the parser understanding the nodes in that line by the method List<InlineBreakdown> lookup(String input);.

So taking this case for example noties/Markwon#67.
The user needs to make the inline parser understanding how to handle this new format ~bug ~"Multiple word fea"

For this one is just needed one extension that based on the line provides the Nodes(or Custom Nodes) with their respective line positions:

InlineParser.NodeExtension nodeExtension = new InlineParser.NodeExtension() {
    @Override
    public List<InlineBreakdown> lookup(String inline) {
        Matcher matcher = MAGIC_PATTERN.matcher(inline);
        List<InlineBreakdown> inlinesBreakdown = new ArrayList<>();
        while (matcher.find()) {
                inlinesBreakdown.add(InlineBreakdown.of(new Image(...), matcher.start(), matcher.end()));
        }
        return inlinesBreakdown;
    }
};
Parser parser = Parser.builder().nodeExtension(nodeExtension).build();
Node document = parser.parse("Some text ~bug ~\"Multiple word fea\"");

There are some conventions in the case of conflicts such as:

• If two nodes are in overlapping each other, the resolution will take the lowest beginIndex and the second it will be ignored;
• If two extensions registered to provide Node with the same beginIndex, the first registered will win respecting the ArrayList sort. (As suggested Make inline parsing extensible #113 we can have an enhancement to set rank by NodeExtension ahead;

Pls, let me know what do you think and about the terminologies that I tried. All feedback is very welcome.

#113

[atlassian-cla-bot]

Hooray! All contributors have signed the CLA.

[samukce]

One simple usage example

https://github.com/atlassian/commonmark-java/blob/f3360bbdf494b5304efbbd262a032211f3edfceb/commonmark/src/test/java/org/commonmark/test/UsageExampleTest.java#L73-L90

[samukce]

Hey @robinst , pls, let me know what do you think about it.

[robinst]

Hey, thanks for raising this. This seems promising, I like the simplicity of the API!

A couple of questions/first impressions:

1. I'm not sure about the naming yet. This seems more like a preprocessor thing, hmmm. Need to think about it more.
2. In parseInline, if a builtin syntax parses something and it overlaps with an extension, it looks like the builtin syntax wins, right? What happens to the extension node in that case? A test for that would be good. A good example would be if the extension matches something that is in code backticks.
3. It seems potentially slow to let extensions parse over the whole inline structure. When I was last thinking about this I was thinking about being able to register extensions in parseInline that get activated on certain trigger characters (similar to the current way the builtin syntax works), and can then do some parsing (or decide not to). It would be similar to custom delimiter processors, except without the constraints of those. Your approach is potentially more powerful though, because it can match on arbitrary regexes. Have you considered this approach? What I like about it is that we could even make the bulitin syntax be implemented that way, similar to how the core block parsers implement the same interface as an extension does.

[colinodell]

We implemented extensible inline parsing in the league/commonmark library for PHP - I can share some thoughts and lessons learned from our initial implementation:

I'm not sure about the naming yet. This seems more like a preprocessor thing, hmmm. Need to think about it more.

I am terrible at naming things, but we went with the concept of having "block parsers" and "inline parsers", the latter of which runs after the block has been parsed.

In parseInline, if a builtin syntax parses something and it overlaps with an extension, it looks like the builtin syntax wins, right? What happens to the extension node in that case? A test for that would be good. A good example would be if the extension matches something that is in code backticks.

We went with a somewhat-naive solution where all parsers (including both builtin and custom inline parsers) register themselves with a given priority - just an arbitrary integer that defaults to 0. If somebody wants their custom inline parser to "win" they have the option to set that priority to a higher number.

Individual parsers are not aware of others - we basically stop at a given point in the line and then ask the relevant parsers one-at-a-time if they can parse the text at the given position; if so, they'll parse whatever they can, otherwise we'll try the next eligible parser. They can choose to parse as much or little as they need to.

This approach seems to work well for parsing CommonMark and GFM.

It seems potentially slow to let extensions parse over the whole inline structure. When I was last thinking about this I was thinking about being able to register extensions in parseInline that get activated on certain trigger characters (similar to the current way the builtin syntax works), and can then do some parsing (or decide not to).

We originally tried regexes but found the performance was indeed very slow, so we proceeded to implement a character-based triggering which helped significantly. Our inline parsers provide a list of characters they are "interested in" - if we stop on one of those characters, we hand control over to them and that's when they'll attempt to run the slower regexes.

The other benefit to the inline parsers defining a list of characters is that you can dynamically build a regex that can quickly skip through plain text and stop where the next possible inline begins.

This overall approach works great in 99% of cases, but we did come across one situation recently where it fell a bit short: thephpleague/commonmark#492 (comment) I'm leaning towards modifying the implementation to allow for both character-based and regex-based hints so we don't lose the performance optimizations.

[samukce]

I'm not sure about the naming yet. This seems more like a preprocessor thing, hmmm. Need to think about it more.

Yeah, it seems a preprocessor thing... I can rename following this concept. It makes more sense to me.

In parseInline, if a builtin syntax parses something and it overlaps with an extension, it looks like the builtin syntax wins, right? What happens to the extension node in that case? A test for that would be good. A good example would be if the extension matches something that is in code backticks.

Good catch. Yeah, I need to cover this behavior. I tend to assume that the extension should win in this case.

It seems potentially slow to let extensions parse over the whole inline structure. When I was last thinking about this I was thinking about being able to register extensions in parseInline that get activated on certain trigger characters (similar to the current way the builtin syntax works), and can then do some parsing (or decide not to).

Yeah, I thought about this trade-off performance/power so I came with this initial idea to give more power to the user side code. But yeah, it could be to much power with a serious performance impact.

It would be similar to custom delimiter processors, except without the constraints of those. Your approach is potentially more powerful though, because it can match on arbitrary regexes. Have you considered this approach? What I like about it is that we could even make the bulitin syntax be implemented that way, similar to how the core block parsers implement the same interface as an extension does

I thought about this idea to have some character to trigger the extension but I thought that we could have a lot of false positives and that not always we do have some specific characteristics that can define the beginning of some specific text. For example, if from the extension it's considered \ that is also the trigger for parseBackslash() we would have some possible conflicts.

This overall approach works great in 99% of cases, but we did come across one situation recently where it fell a bit short: thephpleague/commonmark#492 (comment) I'm leaning towards modifying the implementation to allow for both character-based and regex-based hints so we don't lose the performance optimizations.

Yeah, I guess I came from the other direction first going by powerful then performance side. lol @colinodell But as you both said, it will have performance impact empowered by Number of Extensions. Kind of [line reads] x [Number of Extensions].

@colinodell gave one idea that could be nice which the client could inform one or character sequential, this could be one interesting idea. @robinst Do you think that could be helpful we keep this current approach in this PR where the extension can have the entire line to decide what to do? I will think about the approach to triggered by character/sequence too.

[samukce]

I will run the performance over this scenario to compare the usage of the extension as it is right now and without the extension.

[samukce]

I got interesting numbers here. I tried to build one simple performance test just to compare with/without extension. So, I did narrow: one regular image syntax and one extension node render and the results it looks almost the same.

Benchmark                                 Mode  Cnt   Score   Error  Units
ParseBenchmark.parseImageByNodeExtension    ss   50  33.225 ± 6.245  ms/op
ParseBenchmark.parseImageRegularUsage       ss   50  32.296 ± 4.497  ms/op

Benchmark                                 Mode  Cnt   Score   Error  Units
ParseBenchmark.parseImageByNodeExtension    ss   50  31.688 ± 4.664  ms/op
ParseBenchmark.parseImageRegularUsage       ss   50  35.466 ± 5.394  ms/op

Benchmark                                 Mode  Cnt   Score   Error  Units
ParseBenchmark.parseImageByNodeExtension    ss   50  42.611 ± 9.557  ms/op
ParseBenchmark.parseImageRegularUsage       ss   50  36.177 ± 5.366  ms/op

https://github.com/atlassian/commonmark-java/blob/067fcf111d15617f7bf7e3f062c8a1b2a4ec5424/commonmark-integration-test/src/test/java/org/commonmark/integration/ParseBenchmark.java#L58-L66

[samukce]

Adding a second extension the performance got significant impacted.

Benchmark                                    Mode  Cnt   Score   Error  Units
ParseBenchmark.parseImageByNodeExtension       ss   50  34.466 ± 5.621  ms/op
ParseBenchmark.parseImageByTwoNodeExtension    ss   50  52.019 ± 5.060  ms/op
ParseBenchmark.parseImageRegularUsage          ss   50  33.606 ± 4.941  ms/op

Benchmark                                    Mode  Cnt   Score   Error  Units
ParseBenchmark.parseImageByNodeExtension       ss   50  30.672 ± 4.996  ms/op
ParseBenchmark.parseImageByTwoNodeExtension    ss   50  55.305 ± 6.249  ms/op
ParseBenchmark.parseImageRegularUsage          ss   50  33.529 ± 4.429  ms/op

[samukce]

Given that we may need to prioritize the current nodes + the nodes extended, I'm studying a possible new design to support it given that we may need to put all nodes(configurations) in the same bucket to be able to prioritize accordingly. So the ideas that you both gave will be really helpful. The main points for doing this that I'm thinking is:

• Trigger node creation by the start character;
• Common bucket for nodes configuration;
• One interaction to provide the nodes without the need to interact multiples times per node Extensions.

I hope to commit an idea in the next days to us discuss it and see how the performance is impacted @robinst / @colinodell

[robinst]

(I wrote this reply days ago, but I must have forgotten to submit it and lost it, sorry about that. Typed it again.)

@colinodell thanks for that detailed comment, very helpful!

We originally tried regexes but found the performance was indeed very slow, so we proceeded to implement a character-based triggering which helped significantly. Our inline parsers provide a list of characters they are "interested in" - if we stop on one of those characters, we hand control over to them and that's when they'll attempt to run the slower regexes.

Yes, that's what I was thinking about, as it very closely matches how the existing inline parsing works, see e.g.:

https://github.com/atlassian/commonmark-java/blob/b5d0f398a56ec4f20e02af978b1dd395f1551c99/commonmark/src/main/java/org/commonmark/internal/InlineParserImpl.java#L213-L218

The other benefit to the inline parsers defining a list of characters is that you can dynamically build a regex that can quickly skip through plain text and stop where the next possible inline begins.

Nice idea, yeah we could do something similar.

This overall approach works great in 99% of cases, but we did come across one situation recently where it fell a bit short: thephpleague/commonmark#492 (comment) I'm leaning towards modifying the implementation to allow for both character-based and regex-based hints so we don't lose the performance optimizations.

Our current autolink extension would have the same problem, as it runs after inline parsing on Text nodes too. But as a first step for making inline parsing customizable, I don't think we have to fix this yet.

@samukce replying to this bit:

I thought about this idea to have some character to trigger the extension but I thought that we could have a lot of false positives and that not always we do have some specific characteristics that can define the beginning of some specific text. For example, if from the extension it's considered \ that is also the trigger for parseBackslash() we would have some possible conflicts.

Yeah, the extension mechanism should be able to handle multiple inline parsers that register for the same trigger character. In the above example, the custom one would be allowed to run first (same as with custom block parsers), and decide whether it wants to handle it or not.

@samukce do you feel like working on that? It might seem a bit daunting because it will mean the entire inline parser will change, but it's gonna be a good change. In any case, I think we should do that before we add any regex-based extension mechanism because we'll have a better starting point with it.

[samukce]

Yes @robinst, I agree. I am doing a small POC to us discuss over given that this idea could mean some huge changes in our codebase. I hope until the weekend I commit the code so we can talk about the idea. :)

[robinst]

Hey @samukce! After the recent changes to inline parsing in #180, it should now be fairly simple to make inline parsing extensible. We will just need to add some methods to the builders and make some things public API.

I'll close this PR. Let me know if you're interested in doing that work.

[samukce]

hello @robinst, sorry for so later answer...lately I have been really busy here =/ . I let you know if I have a chance to get back to this one 🤞