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

[Bugfix] Relaxing requirement of anchorSection for headings #403

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Oct 10, 2022

Bug/issue #, if applicable:

Close swiftlang/swift-book#36

Summary

Relaxing requirement of anchorSection for headings.

Swift DocC originally only support H2 and H3 for anchor section.

After this MR, it will expand into all non-H1 headings.

See discussion at swiftlang/swift-book#36 (comment)

Dependencies

None

Testing

  1. See DocumentationNodeTests.swift

  2. Screenshots
    Before this PR:

SCR-20221011-131

After this PR:

SCR-20221011-12o

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 Kyle-Ye force-pushed the bugfix/kyle/reference-limitation branch from 3aee46f to c16faaf Compare October 10, 2022 17:04
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 10, 2022

@swift-ci please test

@Kyle-Ye Kyle-Ye requested a review from krilnon October 10, 2022 17:05
@Kyle-Ye Kyle-Ye self-assigned this Oct 10, 2022
import XCTest

class DocumentationNodeTests: XCTestCase {
func testH4AndUpAnchorSections() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also check the contents of the anchor sections to make sure they match the title in source?

It would also be good to have an integration-style test that verifies that the render JSON contains the right anchors as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll update the test case later and then request a new review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we also check the contents of the anchor sections to make sure they match the title in source?

Done.

It would also be good to have an integration-style test that verifies that the render JSON contains the right anchors as well.

Looks that it would always be emitted to the render JSON even before this PR. Do we still need to check it?
https://github.com/apple/swift-docc/blob/61100a442ff1021405e19de8b4b225e576b19cdd/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift#L48-L50

But I do think we should check if the following article has no problems when rendering

<doc:HeadingTest#Heading2>
<doc:HeadingTest#Heading3>
<doc:HeadingTest#Heading4>
<doc:HeadingTest#Heading5>
<doc:HeadingTest#Heading6>

I have some problems to do the integration test for this. Could you provide some suggestion on this?

func testH4AndUpAnchorSections() throws {
    let articleSource = """
    # HeadingAnchorTest

    ## Heading2

    ### Heading3
    
    #### Heading4
    
    ##### Heading5

    ###### Heading6
    """
    let article = Article(markup: Document(parsing: articleSource, options: []), metadata: nil, redirects: nil, options: [:])
    let (_, bundle, context) = try testBundleAndContext(copying: "TestBundle") { root in
        try articleSource.write(to: root.appendingPathComponent("HeadingAnchorTest.md"), atomically: true, encoding: .utf8)
        try """
        # HeadingAnchorTest2

        <doc:HeadingTest#Heading2>
        <doc:HeadingTest#Heading3>
        <doc:HeadingTest#Heading4>
        <doc:HeadingTest#Heading5>
        <doc:HeadingTest#Heading6>
        """.write(to: root.appendingPathComponent("HeadingAnchorTest2.md"), atomically: true, encoding: .utf8)
    }
    
    // FIXME: Could not get the node from documentationCache
//        let node = try context.entity(with: ResolvedTopicReference(bundleIdentifier: "org.swift.docc.example", path: "/documentation/HeadingAnchorTest", sourceLanguage: .swift))
    let node = try DocumentationNode(reference: ResolvedTopicReference(bundleIdentifier: "org.swift.docc.example", path: "/documentation/HeadingAnchorTest", sourceLanguage: .swift), article: article)
    XCTAssertEqual(node.anchorSections.count, 5)
    for (index, anchorSection) in node.anchorSections.enumerated() {
        let expectedTitle = "Heading\(index + 2)"
        XCTAssertEqual(anchorSection.title, expectedTitle)
        XCTAssertEqual(anchorSection.reference, node.reference.withFragment(expectedTitle))
    }
    let unresolvedTopicProblems = context.problems.filter { $0.diagnostic.identifier == "org.swift.docc.unresolvedTopicReference" }
    // FIXME: This it not going to work since there is other unresolvedTopicReference in TestBundle. How can we get a bundle and context only have this 2 articles?
    XCTAssertEqual(unresolvedTopicProblems.count, 0)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay on this. You're right, since this PR doesn't affect how the headings are propagated through the render JSON, there's no need for an additional text.

@Kyle-Ye Kyle-Ye changed the title Relaxing requirement of anchorSection for headings [Bugfix] Relaxing requirement of anchorSection for headings Oct 10, 2022
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/reference-limitation branch from c16faaf to 352c42d Compare October 16, 2022 17:51
Copy link
Contributor

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Thank you @Kyle-Ye !

import XCTest

class DocumentationNodeTests: XCTestCase {
func testH4AndUpAnchorSections() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay on this. You're right, since this PR doesn't affect how the headings are propagated through the render JSON, there's no need for an additional text.

@franklinsch
Copy link
Contributor

@Kyle-Ye can you please rebase and trigger CI?

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/reference-limitation branch from 352c42d to 3d6c575 Compare November 3, 2022 16:52
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 3, 2022

@swift-ci please test

@Kyle-Ye Kyle-Ye merged commit 6e35675 into swiftlang:main Nov 3, 2022
@Kyle-Ye Kyle-Ye deleted the bugfix/kyle/reference-limitation branch November 3, 2022 16:57
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.

Unresolved H4 links
2 participants