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.

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

Conversation

Konikz
Copy link

@Konikz Konikz commented Apr 4, 2025

Bug/issue #, if applicable: #1111

Summary

This PR addresses issue #1111 by adding a solution to remove invalid metadata directives in documentation comments. When a user encounters a warning about an invalid metadata directive in a documentation comment, they will now see a solution that allows them to remove the directive with a single click, rather than having to manually edit the source code.

Dependencies

None. This PR doesn't depend on any other changes.

Testing

To test this functionality, you can use the following test content:

@Metadata {
    @DisplayName("Custom Name")
    @TechnologyRoot
}

Steps:

  1. Add the test content to a documentation comment in your Swift code.
  2. Build the documentation with DocC.
  3. You should see warnings about invalid metadata directives in documentation comments.
  4. Each warning should have a solution that allows you to remove the directive.
  5. Click on the solution to remove the directive.

The test case testInvalidMetadataDirectivesInDocumentationCommentHaveSolution in Tests/SwiftDocCTests/Semantics/MetadataTests.swift verifies that:

  • The correct number of problems are generated
  • Each problem has a solution
  • The solution has the correct summary and replacement

Checklist

  • Added tests
  • Ran the ./bin/test script and it succeeded ()
  • Updated documentation if necessary

@d-ronnqvist
Copy link
Contributor

@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.

The code generally looks good but there's a compiler error in the added test.

Comment on lines +456 to +457
XCTAssertNotNil(problem.possibleSolutions)
XCTAssertEqual(problem.possibleSolutions?.count, 1)
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.

for problem in problems {
XCTAssertNotNil(problem.possibleSolutions)
XCTAssertEqual(problem.possibleSolutions?.count, 1)
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.)

@Konikz
Copy link
Author

Konikz commented Apr 9, 2025

@d-ronnqvist working on rectifying the issue

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