Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for transforming Doxygen discussion/note tags #798

Merged
merged 10 commits into from
Feb 15, 2024

Conversation

j-f1
Copy link
Contributor

@j-f1 j-f1 commented Jan 12, 2024

Bug/issue #, if applicable: Related to rdar://116077012

Summary

This note adds support for formatting Doxygen @discussion and @note tags. Discussion tags are turned into normal paragraphs, while @note tags are turned into “note” blocks.

Dependencies

Testing

Create a documentation comment containing @note or @discussion tags:

This is an overview.

@discussion This is a discussion.

@note Please take note.

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 — I don’t think this is needed, right?

@j-f1
Copy link
Contributor Author

j-f1 commented Jan 12, 2024

@swift-ci please test

@d-ronnqvist
Copy link
Contributor

Does the reference resolver already visit this content? A good way to check would be to add unresolvable links in both places and look for warnings:

This is an overview.

@discussion This is a <doc:NotFound> discussion.

@note Please take <doc:NotFound> note.

@j-f1
Copy link
Contributor Author

j-f1 commented Jan 17, 2024

Does the reference resolver already visit this content? A good way to check would be to add unresolvable links in both places and look for warnings:

Added that doc comment to a test project and got warnings on both links.

@j-f1
Copy link
Contributor Author

j-f1 commented Jan 17, 2024

@swift-ci please test

@j-f1 j-f1 force-pushed the jed/doxygen-discussion-note branch from 09a2854 to 2e238c1 Compare February 1, 2024 17:42
@j-f1
Copy link
Contributor Author

j-f1 commented Feb 1, 2024

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR.

Can you add some tests that verify:

  • that the content of these tags is available on the in-memory node
  • that these tags renders as expected
  • that links in content within these tags get resolved

Something like this should be able to serve as a starting point for a test.

func testDoxygenDiscussionAndNote() throws {
    let documentationLines: [SymbolGraph.LineList.Line] = """
        This is an abstract.

        @discussion This is a discussion.

        @note This is a note.
        """
        .splitByNewlines
        .enumerated()
        .map { index, line in
            SymbolGraph.LineList.Line(
                text: line,
                range: .init(start: .init(line: 1 + index, character: 1), end: .init(line: 1 + index, character: 1 + line.utf8.count))
            )
        }
    
    let tempURL = try createTempFolder(content: [
        Folder(name: "unit-test.docc", content: [
            JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
                moduleName: "ModuleName",
                symbols: [
                    SymbolGraph.Symbol(
                        identifier: .init(precise: "some-class-id", interfaceLanguage: SourceLanguage.swift.id),
                        names: .init(title: "SomeClass", navigator: nil, subHeading: nil, prose: nil),
                        pathComponents: ["SomeClass"],
                        docComment: .init(documentationLines),
                        accessLevel: .public,
                        kind: .init(parsedIdentifier: .class, displayName: "Kind Display Name"),
                        mixins: [:]
                    )
                ]
            )),
        ])
    ])
    
    let (_, bundle, context) = try loadBundle(from: tempURL)
    let reference = ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: "/documentation/ModuleName/SomeClass", sourceLanguage: .swift)
    
    // Verify the expected content in the in-memory model
    let node = try context.entity(with: reference)
    let symbol = try XCTUnwrap(node.semantic as? Symbol)
    
    XCTAssertEqual(symbol.abstract?.format(), "This is an abstract.")
    XCTAssertEqual(symbol.discussion?.content.map { $0.format() }, ["This is a discussion.", "This is a note."])
    
    // Verify the expected content in the render model
    var translator = RenderNodeTranslator(context: context, bundle: bundle, identifier: node.reference, source: nil)
    let renderNode = try XCTUnwrap(translator.visit(node.semantic) as? RenderNode)
    
    XCTAssertEqual(renderNode.abstract, [.text("This is an abstract.")])
    XCTAssertEqual(renderNode.primaryContentSections.count, 1)
    
    let overviewSection = try XCTUnwrap(renderNode.primaryContentSections.first as? ContentRenderSection)
    XCTAssertEqual(overviewSection.content.count, 3)
    
    XCTAssertEqual(overviewSection.content, [
        .heading(.init(level: 2, text: "Overview", anchor: "overview")),
        
        .paragraph(.init(inlineContent: [
            .text("This is a discussion.")
        ])),
        
        .aside(.init(style: .init(asideKind: .note), content: [
            .paragraph(.init(inlineContent: [
                .text("This is a note.")
            ]))
        ])),
    ])
}

Note that it's just a starting point. It is incomplete in that it doesn't really verify the content of the in-memory node and that it doesn't include any links in the content.

Co-Authored-By: David Rönnqvist <[email protected]>
@j-f1
Copy link
Contributor Author

j-f1 commented Feb 6, 2024

What do you mean by “verify the content of the in-memory node?” Does that mean I should be introspecting symbol.discussion?.content and not just comparing the formatted strings in the test? Added your test + some links in the meantime.

@j-f1
Copy link
Contributor Author

j-f1 commented Feb 6, 2024

@swift-ci please test

@j-f1 j-f1 force-pushed the jed/doxygen-discussion-note branch from 81d113c to 8615023 Compare February 6, 2024 17:11
@j-f1
Copy link
Contributor Author

j-f1 commented Feb 6, 2024

@swift-ci please testr

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

I wonder if needing to explicitly handle DoxygenDiscussion and DoxygenNote when working with the in-memory models could be something that developer forgets and that new features won't automatically be supported in such content but at the moment I don't know of a better way to handle the in-memory models, and that's always a change we can make in the future—for example if we find this to be a problem.

@j-f1
Copy link
Contributor Author

j-f1 commented Feb 15, 2024

@swift-ci please test

@j-f1 j-f1 merged commit bad0aaa into main Feb 15, 2024
2 checks passed
@j-f1 j-f1 deleted the jed/doxygen-discussion-note branch February 15, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants