Add support for custom attributes

[christianselig]

Currently the library will crash if you input Markdown such as the following:

Here is a ^[custom attribute](rainbow: 'extreme')

This is because while the swift-cmark side of things is knowledgeable about custom attributes (also see WWDC21 "What's New in Foundation @ 10:12" for its usage), this library's Swift counterpart to the C code isn't aware of custom attributes, and fatal errors with an unknown type if such an example is encountered. (Ask me how I know!)

This PR adds a CustomAttributes Swift type into the library as well as some functions to get it to work, thereby preventing the crash and letting users implement custom attributes in their Walkers/Visitors/Rewriters.

Some design decisions I wasn't 100% sure of:

• The name CustomAttributes. The plural does seem necessary as the actual custom attributes (per the WWDC video) can be JSON5 with any number of attributes, rather than just 1 in the example above. Custom as a prefix is also debatable, but Attributes felt too generic, and CustomBlock already exists.
• For the user-facing Walker API, I wasn't sure if the CustomAttributes object should have a value that is a String (the raw string/JSON representing the attributes), or a [String: AnyHashable] dictionary where the values are actually broken down. I erred on the side of a string, as it would allow the user to ingest it more flexibly, through a custom Codable implementation, JSONSerialization, etc. rather trivially, instead of the API trying to guess what they want their output as.
• In the same vein, the output for MarkupFormatter could technically have each key-value pair on a separate line to better adhere to preferredLineLimit, however formatting the output as such also gets into the weeds of JSON formatting, proper indentation, and things likely beyond the scope of the formatter so I elected not to.

(Thank you to @QuietMisdreavus for help.)

[esummers]

I'm running in to the same issue (swift-markdown crashing) with my app that is processing markdown containing custom attributes.

I noticed that the commit history of swift-cmark is referring to these as inline directives. Instead of calling it CustomAttributes, maybe it should be called InlineDirective. That would also solve the question of plurality.

[QuietMisdreavus]

cc @parkera in case you have opinions on naming/serialization in Swift-Markdown, or the use of attributes in Swift generally

This looks good so far! Could you add some tests for the custom attribute type? Something like the ones in Tests/MarkdownTests/Inline Nodes/LinkTests.swift should be enough here.

[QuietMisdreavus]

Generally, from the point of view of a generalized Markdown library, i'm fine vending the attribute items as plain text, since that mirrors what the extension in swift-cmark-gfm does. Same for the use of CustomAttribute as the type name. We don't need to do much massaging of the data at this level; presumably consumers of this data will have some expected formats they would want if they wanted to use this feature. (And if they don't, returning the data as plain-text means that it can be reparsed as what they did expect to see!)

[parkera]

Thank you for this PR! I think treating these as String is in the correct spirit (agreeing with @QuietMisdreavus). The separation of concerns we had in mind here was that cmark would think of these just as strings without attempting any higher-level interpretation. It's left up to the caller to figure out how to interpret them (e.g., via parsing them as JSON5 as Foundation does).

As for the name, I think it's fine. One thing to keep in mind, perhaps, is that cmark provides a pretty strong distinction between inline and block elements, and our custom attribute syntax here is entirely for inline values. It may be worth reflecting that in the API.

[QuietMisdreavus]

Circling back to this PR. @christianselig, do you think you'll have time to make some tweaks? If not, let me know and i can push it across the finish line.

One thing to keep in mind, perhaps, is that cmark provides a pretty strong distinction between inline and block elements, and our custom attribute syntax here is entirely for inline values. It may be worth reflecting that in the API.

One easy way to address this would be to call the type "inline attributes" instead of "custom attributes". A lot of the swift-cmark code just uses the name "attribute" when referring to the node kind, but it's always in the context of inline nodes. Some places use "custom attributes", but i think in the context of parsing them out like this, saying "inline attributes" or just "attributes" should be fine.

[christianselig]

@QuietMisdreavus Absolutely can do, I'll get working on this and see to changing "custom attributes" to "inline attributes".

[jurex]

hey guys, any update on this? Would love to see it merged too. Thank you!

[QuietMisdreavus]

@christianselig If you don't have time to come back to this, i can pick it up get it across the finish line.

[christianselig]

So sorry, I'll do it this coming week, this has been at the top of my to do list for awhile but the iOS 16 stuff has commanded a bit more of my time than I originally thought (I'm sure you have a lot on your plate too there!)

[christianselig]

@QuietMisdreavus Should all be finished now, again very sorry for the delay in completing this.

• Renamed CustomAttributes to InlineAttributes
• Added tests for InlineAttributes that mirror Link's

I also attempted to resolve the merge conflict by adding in the new addition to the MarkupTreeDumper file but it looks like you might need to sign off on that.

Let me know if there's anything further I can do and I promise I'll complete it faster this time. :)

[QuietMisdreavus]

Not sure if this comment should be updated, but this file should probably be renamed.

[QuietMisdreavus]

GitHub seems to think that there's still a merge conflict? Not sure if something landed between when you pulled main and now, but you'll need to merge/rebase again.

[christianselig]

Okay, I think I did this correctly despite accidentally closing the PR in the process. 😛 File (and comment) should be renamed properly, and I pulled down the Apple repo fresh so there should be no further merge conflicts.

[QuietMisdreavus]

@swift-ci Please test

[QuietMisdreavus]

This looks great, thanks so much! I double-checked this branch against Swift-DocC and it looks like this doesn't break anything over there, so i'll go ahead and land this. (For the moment docc will skip printing anything for inline attributes it sees, but it's a far cry from crashing! Proper support will require some work on Swift-DocC to determine what we want to do with it.)

[QuietMisdreavus]

Actually, sorry for the back and forth, but could you add some tests? You could drop a single function in a new file Tests/MarkdownTests/Inline Nodes/InlineAttributeTests.swift and base it off some of the other tests there, like in SymbolLinkTests how it parses a Document and checks its debug dump.

[christianselig]

No problem! How'd I do?

[QuietMisdreavus]

Looks good, but i was also hoping to see something that parsed a Document, just because that's the crashing path in Swift-DocC. 😅 Something like this...

func testParseInlineAttributes() {
    let source = "^[Hello, world!](rainbow: 'extreme')"
    let document = Document(parsing: source)
    let expectedDump = """
        Document @1:1-1:37
        └─ Paragraph @1:1-1:37
           └─ InlineAttributes @1:1-1:37 attributes: `rainbow: 'extreme'`
              └─ Text @1:3-1:16 "Hello, world!"
        """
    XCTAssertEqual(expectedDump, document.debugDescription(options: .printSourceLocations))
}

[christianselig]

On it!

[christianselig]

Sorry for the delay here, added that Document-based test (thank you for writing that), if there's any more tests or anything you'd like written just say so and I'll be (more quickly!) on it!

[QuietMisdreavus]

This looks great, thanks so much! Let's land this!

[QuietMisdreavus]

@swift-ci Please test