[robinst]

The end goal is to extract all current inline parsers, and then allow customizing inline parsing via the same infrastructure (similar to the current block parsing).

A stretch goal is to move away from the parser input having to be a single contiguous String. This could speed up parsing and allow keeping track of source positions (line and column indexes) for inline nodes.

Still todo:

• Replace remaining direct usages of index and match in InlineParserImpl with Scanner
• See if we can change input from String to List<CharSequence>
• Clean up internal API

(Allowing extension of inline parsers can be done in a separate PR, but it will be much simpler now.)

[samukce]

This scanner class helped a lot the code readability. :)

[robinst]

Yeah, I think so too :).

[samukce]

I can see we using this symbol in multiple places. What do you think to extract and also use the constant from Character class? Character.MIN_VALUE?

[robinst]

Good idea, yeah! Character.MIN_VALUE would work, but I think something even more specific would be nicer like EOF or END (in Scanner or Parsing).

[samukce]

While reading the code this next method makes me think a different action for this method. When I renamed to isNext(...) it more senses to me. what do you think?

[robinst]

Yeah, it's really a nextIf. What I like about next is that it reads well in code like if (scanner.next('x')).

With isNext it's not clear that it advances the scanner state to me.

[samukce]

The Builderclass could have this boot.

return new Builder();
...
 private Builder() {
            this.set = new BitSet();
        }

[samukce]

To have an overload next(...) method for multiple char could increase even more the readability here.

Something like

    if (scanner.next('C', 'D', 'A', 'T', 'A', '[') {
   ....

    //Scanner.java
    public boolean next(char... c) {
        for (char character: c) {
            if (!next(character)){
                return false;
            }
        }
        return true;
    }

   //ScannerTest.java
    @Test
    public void testIsNextMultipleCharacter() {
        Scanner scanner = new Scanner(Collections.<CharSequence>singletonList("foo bar"), 0, 0);
        assertTrue(scanner.next('f', 'o', 'o', ' ', 'b', 'a', 'r'));
        assertFalse(scanner.next('r'));
    }

[robinst]

Yeah, I was considering this before, maybe even a next(String) instead, which only advances if it finds the whole string.

[samukce]

Just to simplify those if/else and have less indentation.

Suggested change

	if (next == '\n') {
	scanner.next();
	return ParsedInline.of(new HardLineBreak(), scanner.position());
	} else if (ESCAPABLE.matcher(String.valueOf(next)).matches()) {
	scanner.next();
	return ParsedInline.of(new Text(String.valueOf(next)), scanner.position());
	} else {
	return ParsedInline.of(new Text("\\"), scanner.position());
	}
	if (next == '\n') {
	scanner.next();
	return ParsedInline.of(new HardLineBreak(), scanner.position());
	}
	if (ESCAPABLE.matcher(String.valueOf(next)).matches()) {
	scanner.next();
	return ParsedInline.of(new Text(String.valueOf(next)), scanner.position());
	}
	return ParsedInline.of(new Text("\\"), scanner.position());

[samukce]

We do have an attribute in this line called line too. We may call here by simple i,j,....

[samukce]

Suggested change

	throw new IllegalArgumentException("Can only match ASCII characters");
	throw new IllegalArgumentException("Can only match ASCII Printable Characters");

[robinst]

I'm happy with this PR now (for internal API, would still need some more cleanup to be public API), but I still want to spike what this type would look like with source positions, so that we can avoid breaking API twice.