add optional Aside intiailizer that bails if it can't find a tag

[QuietMisdreavus]

Bug/issue #, if applicable: Resolves #139

Summary

This PR adds a new initializer to Aside that only returns a new instance if it successfully finds an aside tag at the beginning of the block-quote. To make the API fully flexible, this API introduces three separate modes to this initializer:

1. tagNotRequired, which defers to the currently-available initializer that creates a .note aside if it can't find a tag,
2. requireSingleWordTag, which requires that the block starts with a single-word tag (and is used by the new HTML formatter), and
3. requireAnyLengthTag, which requires that the block starts with a tag (defined as any text prefixed by a colon), even tags with multiple words like See Also:.

Dependencies

None

Testing

As this is an API-only change, the testing strategy is limited to automated testing.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary

[QuietMisdreavus]

@swift-ci Please test

[d-ronnqvist]

👍 I like having the 3rd "no requirement" be an enum case like this rather than a nil value because it allows us to document the case and make it more explicit at the call site.

[d-ronnqvist]

There's a bug where the parsed aside doesn't have the source location of the original block quote

[d-ronnqvist]

very minor: If you move the firstIndex(of:) check up here, then you can avoid checking the text twice and avoid force unwrapping below.

Suggested change

	]) as? Text, initialText.string.contains(":") else {
	]) as? Text, let firstColonIndex = initialText.string.firstIndex(of: ":") else {

[d-ronnqvist]

just curious: when is initialText.parent!.parent! not the same as self?

[QuietMisdreavus]

I feel like it should always be the same, but i copied the existing code over in case it was different. I'll translate it to use self instead and check that it still works.

[d-ronnqvist]

I think the aside parsing is losing the source URL location, if the original markup had any.

If I change this function to

+ let fakeFileLocation = URL(fileURLWithPath: "/path/to/some-file.md")
  let document = Document(parsing: source, source: fakeFileLocation)
  let blockQuote = document.child(at: 0) as! BlockQuote
  let aside = try XCTUnwrap(Aside(blockQuote, tagRequirement: conversionStrategy), file: file, line: line)
+ XCTAssertEqual(
+     blockQuote.range?.lowerBound.source,
+     aside.content.first?.range?.lowerBound.source,
+     "The parsed aside shouldn't lose its source information",
+     file: file, line: line
+ )
  XCTAssertEqual(expectedKind, aside.kind, file: file, line: line)
- XCTAssertEqual(expectedRootDump, aside.content[0].root.debugDescription(), file: file, line: line)
+ XCTAssertEqual(expectedRootDump, aside.content[0].root.debugDescription(options: .printSourceLocations), file: file, line: line)

I get failures for every call site that "Optional(file:///path/to/some-file.md)") is not equal to ("nil")

Also, the expected root dumps all pass, despite dumping the aside with debugDescription(options: .printSourceLocations).

I think together these mean that the aside's markup elements don't have a source location anymore.

The effect of this will be that other tools can't present diagnostics about the content of the parsed aside and point to the location in the source file.

[d-ronnqvist]

Following up on this comment:
If I add these assertions to the code and run the tests, then I hit the 2nd assertion (and 3rd assertion if I comment out the 2nd).

+ assert(initialText.range?.lowerBound.source == self.range?.lowerBound.source, "Parsing didn't lose the original source information")
  initialText.string = String(trimmedText)
+ assert(initialText.range?.lowerBound.source == self.range?.lowerBound.source, "Parsing didn't lose the original source information")
  let newBlockQuote = initialText.parent!.parent! as! BlockQuote
+ assert(newBlockQuote.range?.lowerBound.source == self.range?.lowerBound.source, "Parsing didn't lose the original source information")

[QuietMisdreavus]

It looks like the initialText.string = String(trimmedText) line is clearing the range for the text node, which in turn is clearing the range for the block quote as a whole. This bug was present in the old version of the code for any Asides that are parsed, since this code was adapted from the existing Aside initializer, but we can fix it for the future.

[d-ronnqvist]

Do you know if it's easy to fix? I'm fine with fixing it later. I mainly don't want to lose track of the information in this PR since we've already identified which line causes the bug.

[QuietMisdreavus]

It involves doing a lot of tree copies since the source range is part of the RawMarkupHeader type. I believe i have a fix written on my machine, i just need to make sure it works as expected.

[QuietMisdreavus]

I pushed up a commit that preserves the parsed range in Asides' BlockQuotes.

I took the time to modify the RawMarkup replacingSelf/substitutingChild methods to allow for preserving the parents' ranges. This allows the BlockQuote itself to reference the span including the first > character, the enclosing paragraph to reference the whole block including the kind tag, and the first line of the parsed aside to only reference the text after the kind tag. I'm unsure about preserving all the ranges like this, but i felt like it was better to have it this way instead of having the parent BlockQuote and Paragraph (not even to mention the whole document) have a shifted starting point.

[QuietMisdreavus]

@swift-ci Please test

[d-ronnqvist]

Question: what does it mean to pass [0, 0] here?

[QuietMisdreavus]

It's a child path, in this case the first child of the first child. The descendant that is pointed to by this path will be substituted by the given markup data, and then the ancestors of that node will be updated/copied to reflect their new children.

[d-ronnqvist]

minor: column (which this adds to) is documented to be number of UTF-8 bytes. It's possible that a tag name in another language could cause an offset source range.

Suggested change

	let shiftCount = kindTag.count + 1 + initialText.string[firstColonIndex...].dropFirst().prefix(while: {
	let shiftCount = kindTag.utf8.count + 1 + initialText.string[firstColonIndex...].dropFirst().prefix(while: {

[QuietMisdreavus]

@swift-ci Please test