Skip to content

@DeprecationSummary not having effect for some symbols #982

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

Merged

Conversation

anferbui
Copy link
Contributor

@anferbui anferbui commented Jul 11, 2024

Bug/issue #, if applicable: Closes #932, rdar://70056350.

Summary

Fixes a bug where the @DeprecationSummary directive would have no effect if a symbol had multiple lines of documentation comments in its source, like so:

/// I am adding some amazing documentation for my new struct.
///
/// ## Overview
///
/// In fact it's so cool, that it even needs some markdown.
public struct ExampleDeprecated {}

The root cause of the issue was that the @DeprecatedSummary directive would only take effect when used in the "Abstract" section of the markup.

When using the default merging strategy for documentation extension files (append), documentation extension content is added after all the existing documentation comments, resulting in something like:

I am adding some amazing documentation for my new struct.

## Overview

In fact it's so cool, that it even needs some markdown.

@DeprecationSummary {
    This symbol is deprecated.
}

This would make the @DeprecationSummary directive be in the Discussion section, not the Abstract, so it would be ignored. This made it impossible to override the deprecation summary from the documentation extension file for any symbols which had multiple lines of documentation comments. (It worked fine if the symbol has no source documentation or only one line of source documentation).

This PR makes it so that the @DeprecationSummary can be parsed in both the Abstract and Discussion sections.

Dependencies

N/A.

Testing

You need the following setup:

  • A symbol with multiple lines of documentation in the source code
  • A documentation extension file which uses the @DeprecationSummary directive, for example.

Example DocC bundle:
TestPackage.docc.zip

Steps:

  1. Download example bundle
  2. Run docc preview TestPackage.docc
  3. Go to http://localhost:8080/documentation/testpackage/exampledeprecated and verify that the symbol is marked as deprecated.

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 Not necessary

Verification

Unit testing
Added unit tests to DocumentationMarkupTests and DeprecationSummaryTests .

[1703/1703] Testing SwiftDocCTests.SynchronizationTests/testBlockingWithLock
=> Checking for unacceptable language… okay.
=> Checking license headers… okay.
=> Validating scripts in bin subdirectory… okay

Site rendering

Before After
Screenshot 2024-07-11 at 2 50 47 PM Screenshot 2024-07-11 at 2 51 23 PM

anferbui added 4 commits July 11, 2024 12:06
When specifying the @DeprecationSummary directive in a symbol extension markdown file, the directive wasn't taking effect if the symbol had multiple lines of documentation comments.

This was happening because `DocumentationMarkup.deprecation` was only being populated in the abstract. I've now updated it to take an effect in the Discussion section as well.

Resolves rdar://70056350.
Resolves rdar://70056350.
When we've parsed the `@DeprecationSummary` directive, no need to continue parsing, we can exit early.

Resolves rdar://70056350.
@anferbui
Copy link
Contributor Author

@swift-ci please test

anferbui added a commit to anferbui/swift-docc that referenced this pull request Jul 11, 2024
While looking at swiftlang#982, I found that there's a better place to keep the list of directive names which we consider removed from content -- we already had a file with lists of directive names so it'll be clearer to keep all of them in the same file.
@anferbui anferbui mentioned this pull request Jul 11, 2024
1 task
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.

LGTM

Comment on lines 234 to 237
guard let renderNode = translator.visit(symbol) as? RenderNode else {
XCTFail("Could not compile the node")
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: using XCTUnwrap is a more succinct way to assert that a value is non-nil

Suggested change
guard let renderNode = translator.visit(symbol) as? RenderNode else {
XCTFail("Could not compile the node")
return
}
let renderNode = try XCTUnwrap(translator.visit(symbol) as? RenderNode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I copy&pasted previous tests and failed to notice this. Reworked the whole file to use XCTUnwrap where appropriate!

anferbui added 2 commits July 11, 2024 16:57
`XCTUnwrap` is preferred over force-unwrapping or using `XCTFail` in tests.
It's not needed, `internal` is the default access level.
@anferbui
Copy link
Contributor Author

@swift-ci please test

@anferbui anferbui merged commit e54de2e into swiftlang:main Jul 11, 2024
2 checks passed
@anferbui anferbui deleted the deprecated-summary-not-having-effect branch July 11, 2024 16:08
anferbui added a commit to anferbui/swift-docc that referenced this pull request Jul 11, 2024
When specifying the @DeprecationSummary directive in a symbol extension markdown file, the directive wasn't taking effect if the symbol had multiple lines of documentation comments.

This was happening because `DocumentationMarkup.deprecation` was only being populated in the abstract. This has now been updated to take an effect in the Discussion section as well.

* Make `@DeprecationSummary` have an effect in the Discussion section
* Add tests to verify `@DeprecationSummary` has effect in Discussion section
* Return early when `@DeprecationSummary` is detected. When we've parsed the `@DeprecationSummary` directive, no need to continue parsing, we can exit early.
* Use `XCTUnwrap` in DeprecationSummaryTests. `XCTUnwrap` is preferred over force-unwrapping or using `XCTFail` in tests.
* Remove `internal` keyword. It's not needed, `internal` is the default access level.

Resolves rdar://70056350.
anferbui added a commit to anferbui/swift-docc that referenced this pull request Jul 12, 2024
When specifying the @DeprecationSummary directive in a symbol extension markdown file, the directive wasn't taking effect if the symbol had multiple lines of documentation comments.

This was happening because `DocumentationMarkup.deprecation` was only being populated in the abstract. This has now been updated to take an effect in the Discussion section as well.

* Make `@DeprecationSummary` have an effect in the Discussion section
* Add tests to verify `@DeprecationSummary` has effect in Discussion section
* Return early when `@DeprecationSummary` is detected. When we've parsed the `@DeprecationSummary` directive, no need to continue parsing, we can exit early.
* Use `XCTUnwrap` in DeprecationSummaryTests. `XCTUnwrap` is preferred over force-unwrapping or using `XCTFail` in tests.
* Remove `internal` keyword. It's not needed, `internal` is the default access level.

Resolves rdar://70056350.
anferbui added a commit that referenced this pull request Jul 12, 2024
When specifying the @DeprecationSummary directive in a symbol extension markdown file, the directive wasn't taking effect if the symbol had multiple lines of documentation comments.

This was happening because `DocumentationMarkup.deprecation` was only being populated in the abstract. This has now been updated to take an effect in the Discussion section as well.

* Make `@DeprecationSummary` have an effect in the Discussion section
* Add tests to verify `@DeprecationSummary` has effect in Discussion section
* Return early when `@DeprecationSummary` is detected. When we've parsed the `@DeprecationSummary` directive, no need to continue parsing, we can exit early.
* Use `XCTUnwrap` in DeprecationSummaryTests. `XCTUnwrap` is preferred over force-unwrapping or using `XCTFail` in tests.
* Remove `internal` keyword. It's not needed, `internal` is the default access level.

Resolves rdar://70056350.
anferbui added a commit that referenced this pull request Aug 5, 2024
* Move `directivesRemovedFromContent` to `BlockDirective`

While looking at #982, I found that there's a better place to keep the list of directive names which we consider removed from content -- we already had a file with lists of directive names so it'll be clearer to keep all of them in the same file.

* Add missing directive names to `BlockDirective.allKnownDirectiveNames`

A lot of directive names were missing from `BlockDirective.allDirectiveNames`. I've updated the array to include all the missing directives which I could find.

This property is used to throw warnings about directives which are not supported in symbol source documentation.

* Remove duplicated `directivesRemovedFromContent` definition

`directivesRemovedFromContent` was being defined in both `DocumentationMarkup` and `BlockDirective`, removed the duplicate definition.
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.

@DeprecationSummary directive doesn't always have an effect
2 participants