Add link title support for commonmark

[Kyle-Ye]

Bug/issue #, if applicable: Close #67

Summary

Add link title support for CommonMark

Dependencies

None

Testing

See new test case in LinkTests.swift

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

[Kyle-Ye]

Actually, I expect this to be

Suggested change

	XCTAssertEqual(linkWithoutTitle.title, "")
	XCTAssertEqual(linkWithoutTitle.title, nil)

But that's how destination and title is computed.

cmark_node_get_title and cmark_node_get_url will use cmark_chunk_to_cstr which will produce an empty string for a nil input.

We can fix it by updating the getter logic on Link.title. The question is should we update the destination's getter accordingly? If so, it may break the downstream since it was a public symbol.

/// The link's title.
var title: String? {
    get {
        guard case let .link(_, title) = _data.raw.markup.data else {
            fatalError("\(self) markup wrapped unexpected \(_data.raw)")
        }
		if let t = title, t.isEmpty {
			return nil
		} else {
	        return title
		}
    }
	...
}

[d-ronnqvist]

Alternatively we could check if the cmark_node_get_title value was empty in CommonMarkConverter.swift before passing the title to the MarkupConversion initializer

[d-ronnqvist]

But I agree that I would also expect nil here

[Kyle-Ye]

Both implementations are fine for me. The only question is should we sync such logic to Link.destination?

[Kyle-Ye]

Alternatively we could check if the cmark_node_get_title value was empty in CommonMarkConverter.swift before passing the title to the MarkupConversion initializer

+1 for this since we've already similar operations for language.

https://github.com/apple/swift-markdown/blob/07da205e1fc488d88609fea5da1ca227efc6c000/Sources/Markdown/Parser/CommonMarkConverter.swift#L329

[Kyle-Ye]

Done with 2766399

[Kyle-Ye]

@swift-ci please test

[d-ronnqvist]

This seems good to me but I'm not familiar enough with the details to know the potential pitfalls.

[Kyle-Ye]

This seems good to me but I'm not familiar enough with the details to know the potential pitfalls.

Got it. Thanks for your review. Maybe we can wait for @QuietMisdreavus 's review suggestion.

[d-ronnqvist]

This seems good to me but I'm not familiar enough with the details to know the potential pitfalls.

Got it. Thanks for your review. Maybe we can wait for @QuietMisdreavus 's review suggestion.

I would be fine with merging this and addressing any concerns from @QuietMisdreavus later, if there are any.

[Kyle-Ye]

@swift-ci please test

[Kyle-Ye]

@swift-ci please test

[Kyle-Ye]

This seems good to me but I'm not familiar enough with the details to know the potential pitfalls.

Got it. Thanks for your review. Maybe we can wait for @QuietMisdreavus 's review suggestion.

I would be fine with merging this and addressing any concerns from @QuietMisdreavus later, if there are any.

Ping @QuietMisdreavus

[QuietMisdreavus]

Looks good, thanks!

[franklinsch]

It looks like this is breaking tests in Swift-DocC: https://ci.swift.org/job/oss-swift-package-amazon-linux-2/2319/consoleText

@Kyle-Ye Are you able to take a look?

[franklinsch]

I will revert this change to unblock Swift toolchains from being built and we can land this change back alongside the Swift-DocC changes once they're ready.

[Kyle-Ye]

It looks like this is breaking tests in Swift-DocC: ci.swift.org/job/oss-swift-package-amazon-linux-2/2319/consoleText

@Kyle-Ye Are you able to take a look?

Got it. I'll look at it now. @franklinsch

[Kyle-Ye]

It looks like this is breaking tests in Swift-DocC: ci.swift.org/job/oss-swift-package-amazon-linux-2/2319/consoleText

@Kyle-Ye Are you able to take a look?

The breaking is expected. See discussion here #140 (comment)

Sorry I forgot to update the test case for swift-docc.

Here is the fix PR: swiftlang/swift-docc#708