-
Notifications
You must be signed in to change notification settings - Fork 128
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
Support @Metadata
and @DeprecationSummary
in documentation comments
#1107
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2024 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
import Foundation | ||
import Markdown | ||
|
||
/// A utlity type for parsing directives from markup. | ||
struct DirectiveParser<Directive: AutomaticDirectiveConvertible> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: If this doesn't have any state, should it be an enum with static methods instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I typically prefer set these as structs in case we want to add configuration arguments to the initialiser, e.g., to customise the parsing behaviour. In this case it doesn't matter though, because the API is not public and we can switch it from an enum to a struct if/when we need to. |
||
|
||
/// Returns a directive of the given type if found in the given sequence of markup elements and the remaining markup. | ||
/// | ||
/// If there are multiple instances of the same directive type, this functions returns the first instance | ||
/// and diagnoses subsequent instances. | ||
func parseSingleDirective( | ||
_ directiveType: Directive.Type, | ||
from markupElements: inout [any Markup], | ||
parentType: Semantic.Type, | ||
source: URL?, | ||
bundle: DocumentationBundle, | ||
context: DocumentationContext, | ||
problems: inout [Problem] | ||
) -> Directive? { | ||
let (directiveElements, remainder) = markupElements.categorize { markup -> Directive? in | ||
guard let childDirective = markup as? BlockDirective, | ||
childDirective.name == Directive.directiveName | ||
else { | ||
return nil | ||
} | ||
return Directive( | ||
from: childDirective, | ||
source: source, | ||
for: bundle, | ||
in: context, | ||
problems: &problems | ||
) | ||
} | ||
|
||
let directive = directiveElements.first | ||
|
||
for extraDirective in directiveElements.dropFirst() { | ||
problems.append( | ||
Problem( | ||
diagnostic: Diagnostic( | ||
source: source, | ||
severity: .warning, | ||
range: extraDirective.originalMarkup.range, | ||
identifier: "org.swift.docc.HasAtMostOne<\(parentType), \(Directive.self)>.DuplicateChildren", | ||
summary: "Duplicate \(Metadata.directiveName.singleQuoted) child directive", | ||
explanation: nil, | ||
notes: [] | ||
), | ||
possibleSolutions: [] | ||
) | ||
) | ||
} | ||
|
||
markupElements = remainder | ||
|
||
return directive | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -192,5 +192,52 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible { | |||||||||||||
|
||||||||||||||
return true | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Validates the use of this Metadata directive in a documentation comment. | ||||||||||||||
/// | ||||||||||||||
/// Some configuration options of Metadata are invalid in documentation comments. This function | ||||||||||||||
/// emits warnings for illegal uses and sets their values to `nil`. | ||||||||||||||
func validateForUseInDocumentationComment( | ||||||||||||||
symbolSource: URL?, | ||||||||||||||
problems: inout [Problem] | ||||||||||||||
) { | ||||||||||||||
let invalidDirectives: [(any AutomaticDirectiveConvertible)?] = [ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better to have a list of supported directives rather than the unsupported directives here? I imagine that that would need to be updated less frequently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth on this. If we were to specify the valid ones, we'd still need to somehow get the values of the invalid ones in order to produce a diagnostic. I couldn't find a good way to get an array of the valid ones easily. We could maybe use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of having the valid ones, If not we will have to remember to add it to the list, which could be forgotten and lead to errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd need to list the valid ones as well anyway, unfortunately. |
||||||||||||||
documentationOptions, | ||||||||||||||
technologyRoot, | ||||||||||||||
displayName, | ||||||||||||||
callToAction, | ||||||||||||||
pageKind, | ||||||||||||||
_pageColor, | ||||||||||||||
titleHeading, | ||||||||||||||
] + (redirects ?? []) | ||||||||||||||
+ supportedLanguages | ||||||||||||||
+ pageImages | ||||||||||||||
Comment on lines
+211
to
+214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the change on code style here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you clarify what you mean here? This is all new code |
||||||||||||||
|
||||||||||||||
let namesAndRanges = invalidDirectives | ||||||||||||||
.compactMap { $0 } | ||||||||||||||
.map { (type(of: $0).directiveName, $0.originalMarkup.range) } | ||||||||||||||
Comment on lines
+217
to
+218
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. very minor: This is needlessly iterating over the elements twice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opted for iterating twice just to make the code a bit nicer so that we don't have to unwrap
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was more thinking that they could be both done in the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I meant to use |
||||||||||||||
|
||||||||||||||
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) | ||||||||||||||
) | ||||||||||||||
) | ||||||||||||||
} | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
documentationOptions = nil | ||||||||||||||
technologyRoot = nil | ||||||||||||||
displayName = nil | ||||||||||||||
pageKind = nil | ||||||||||||||
_pageColor = nil | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,9 @@ import Markdown | |
/// } | ||
/// ``` | ||
/// | ||
/// You can use the `@DeprecationSummary` directive top-level in both articles and documentation extension files. | ||
/// You can use the `@DeprecationSummary` directive top-level in articles, documentation extension files, or documentation comments. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: you need to run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, thanks. I updated this in the latest commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would also be good to add an "Earlier Versions" aside, like on the @Available documentation to say that before Swift-DocC 6.2 (or 6.1 if we plan on cherry-picking this), @DeprecationSummary isn't supported in documentation comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Updated, thanks! I also realised that |
||
/// | ||
/// > Earlier versions: Before Swift-DocC 6.1, `@DeprecationSummary` was not supported in documentation comments. | ||
/// | ||
/// > Tip: | ||
/// > If you are writing a custom deprecation summary message for an API or documentation page that isn't already deprecated, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the
bundle
andcontext
parameters here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass them all the way down to
DirectiveParser
because theMetadata
initialiser needs it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Franklin and I talked about this some more offline. Ideally the directives wouldn't need the full context because it creates a cyclic dependency. They really only need some mechanism to load data but changing that is way out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this. I think there should be a better way to do it without having to pass the full context and bundle. This does not look correct for initialising a symbol.