Skip to content

Add solutions to remove invalid metadata directives in documentation comments #1277

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions Sources/SwiftDocC/Semantics/Metadata/Metadata.swift
Original file line number Diff line number Diff line change
Expand Up @@ -223,18 +223,38 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {
.map { (type(of: $0).directiveName, $0.originalMarkup.range) }

problems.append(
contentsOf: namesAndRanges.map { (name, range) in
Problem(
contentsOf: namesAndRanges.compactMap { (name, range) in
Copy link
Contributor

Choose a reason for hiding this comment

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

Question; why change this to compactMap? At a glance I don't see optional return values in this closure.

Copy link
Author

Choose a reason for hiding this comment

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

That was a mistake, good catch I fixed it!

guard let range = range else {
// If we don't have a range, we can't provide a solution, so just create a problem without solutions
return Problem(
diagnostic: Diagnostic(
source: symbolSource,
severity: .warning,
range: range,
identifier: "org.swift.docc.\(Metadata.directiveName).Invalid\(name)InDocumentationComment",
summary: "Invalid use of \(name.singleQuoted) directive in documentation comment; configuration will be ignored",
explanation: "Specify this configuration in a documentation extension file"
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: It would be nice to avoid repeating this code (especially the user-facing string that are most likely to be updated in the future). Otherwise there's a risk that someone improves the summary or explanation of this warnings but only makes the change in one of these places.

You can avoid that repetition by only creating the [Solution], not the full Problem, inside the if let range check. If the range is nil you can fall back to []. That gives you a value in both code paths of the if-else-statement that you can pass to the only code path that creates this diagnostic/problem.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it here: d095925

}

let solution = Solution(
summary: "Remove invalid \(name.singleQuoted) directive",
replacements: [
Replacement(range: range, replacement: "")
]
)

return Problem(
diagnostic: Diagnostic(
source: symbolSource,
severity: .warning,
range: range,
identifier: "org.swift.docc.\(Metadata.directiveName).Invalid\(name)InDocumentationComment",
summary: "Invalid use of \(name.singleQuoted) directive in documentation comment; configuration will be ignored",
explanation: "Specify this configuration in a documentation extension file"

// TODO: It would be nice to offer a solution here that removes the directive for you (#1111, rdar://140846407)
)
),
possibleSolutions: [solution]
)
}
)
Expand Down
43 changes: 43 additions & 0 deletions Tests/SwiftDocCTests/Semantics/SymbolTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1311,6 +1311,19 @@ class SymbolTests: XCTestCase {
"org.swift.docc.Metadata.InvalidRedirectedInDocumentationComment",
]
)

// Verify that each problem has exactly one solution to remove the directive
for problem in problems {
XCTAssertEqual(problem.possibleSolutions.count, 1, "Each invalid metadata directive should have exactly one solution")

let solution = try XCTUnwrap(problem.possibleSolutions.first)
XCTAssertTrue(solution.summary.hasPrefix("Remove invalid"), "Solution summary should start with 'Remove invalid'")
XCTAssertEqual(solution.replacements.count, 1, "Solution should have exactly one replacement")

let replacement = try XCTUnwrap(solution.replacements.first)
XCTAssertEqual(replacement.replacement, "", "Replacement should be empty string to remove the directive")
XCTAssertNotNil(replacement.range, "Replacement should have a valid range")
Copy link
Contributor

Choose a reason for hiding this comment

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

minor (non-blocking): It might be worth verifying that the range covers the expected portion of the content.

One way to check that could be to compare it against the range of the problem itself.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it here: 6af147f

}
}

func testParsesDeprecationSummaryDirectiveFromDocComment() async throws {
Expand Down Expand Up @@ -1352,6 +1365,36 @@ class SymbolTests: XCTestCase {
XCTAssert(problems.isEmpty)
}

func testSolutionForInvalidMetadataDirectiveRemovesDirective() async throws {
let (_, problems) = try await makeDocumentationNodeForSymbol(
docComment: """
The symbol's abstract.

@Metadata {
@DisplayName("Invalid Display Name")
}
""",
articleContent: nil
)

XCTAssertEqual(problems.count, 1)
let problem = try XCTUnwrap(problems.first)

XCTAssertEqual(problem.diagnostic.identifier, "org.swift.docc.Metadata.InvalidDisplayNameInDocumentationComment")
XCTAssertEqual(problem.possibleSolutions.count, 1)

let solution = try XCTUnwrap(problem.possibleSolutions.first)
XCTAssertEqual(solution.summary, "Remove invalid 'DisplayName' directive")
XCTAssertEqual(solution.replacements.count, 1)

let replacement = try XCTUnwrap(solution.replacements.first)
XCTAssertEqual(replacement.replacement, "")
XCTAssertNotNil(replacement.range)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Since this is checking a single specific directive it's easier to verify that expected range here than in the other test that loops over list of warnings.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed: 6af147f


// The replacement should remove the entire @DisplayName directive
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: are these comments referring to the test assertions above? If so, you can move them each into the "message" of the respective test assertions above.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed: 8cec277

// This would effectively remove the line "@DisplayName("Invalid Display Name")"
}

// MARK: - Leading Whitespace in Doc Comments

func testWithoutLeadingWhitespace() {
Expand Down