Skip to content

Add solution to remove invalid metadata directives in documentation c… #1189

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.

Sign up for GitHub

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 1 commit into
base: main
Choose a base branch
from
Open

Add solution to remove invalid metadata directives in documentation c… #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 13 additions & 11 deletions Sources/SwiftDocC/Semantics/Metadata/Metadata.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,20 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {

problems.append(
contentsOf: namesAndRanges.map { (name, range) in
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)
)
let 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"
)

let solutions = range.map {
[Solution(summary: "Remove this \(name.singleQuoted) directive.", replacements: [Replacement(range: $0, replacement: "")])]
} ?? []

return Problem(diagnostic: diagnostic, possibleSolutions: solutions)
}
)

Expand Down
27 changes: 27 additions & 0 deletions Tests/SwiftDocCTests/Semantics/MetadataTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,5 +433,32 @@ class MetadataTests: XCTestCase {

return (problemIDs, metadata)
}

func testInvalidMetadataDirectivesInDocumentationCommentHaveSolution() throws {
let source = """
@Metadata {
@DisplayName("Custom Name")
@TechnologyRoot
}
"""
let document = Document(parsing: source, options: .parseBlockDirectives)
let directive = document.child(at: 0)! as! BlockDirective
let (bundle, _) = try testBundleAndContext()
var problems = [Problem]()
let metadata = Metadata(from: directive, source: nil, for: bundle, problems: &problems)

metadata?.validateForUseInDocumentationComment(symbolSource: nil, problems: &problems)

XCTAssertEqual(problems.count, 2)

// Verify that each problem has a solution to remove the directive
for problem in problems {
XCTAssertNotNil(problem.possibleSolutions)
XCTAssertEqual(problem.possibleSolutions?.count, 1)
Comment on lines +456 to +457
Copy link
Contributor

Choose a reason for hiding this comment

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

possibleSolutions is non-optional so the XCTAssertNotNil check isn't necessary and the optional chaining in the other assertions fail to compile.

XCTAssertEqual(problem.possibleSolutions?.count, 1)
                                        `- error: cannot use optional chaining on non-optional value of type '[Solution]'

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: since you're checking a few different values of the same first solution, you can require that the first solution exist once using XCTUnwrap and use the unwrapped value in the later test assertions.

However, this is just a matter of style and preference. Not a requested change. I'm happy with both styles here, whichever you find more readable.

XCTAssertEqual(problem.possibleSolutions?.first?.summary, "Remove this \(problem.diagnostic.identifier.split(separator: ".").last!) directive.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this test assertion will fail because the string it's expecting doesn't put single quotes around the directive name like the solution does.

FYI: A completely different style that some will find more readable and some will find less readable could be to move the summary assertion out of the for-loop and assert on both messages together:

XCTAssertEqual(problems.map(\.possibleSolutions.first?.summary), [
    "Remove this 'DisplayName' directive.",
    "Remove this 'TechnologyRoot' directive.",
])

(I believe that the order of the problems will be stable based on the order that they appear in the test content. Otherwise you can unwrap the first solution and sort the mapped summaries.)

XCTAssertEqual(problem.possibleSolutions?.first?.replacements.count, 1)
XCTAssertEqual(problem.possibleSolutions?.first?.replacements.first?.replacement, "")
}
}
}