-
Notifications
You must be signed in to change notification settings - Fork 166
RenderBlockContent.Aside Names #1361
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
base: main
Are you sure you want to change the base?
Conversation
`name` to stored the name or title of aside blocks. This allows DocC to independently save and control the styling and the name of asides. Previously DocC conflated the style of the aside block (e.g. colors or other styling) with the name of the aside. Although it isn't currently possible to author an aside with a different style and name, this change allows the `Aside` model itself to support that one day. Aside now has four available initializers: * init(style: AsideStyle, content: [RenderBlockContent]) * init(name: String, content: [RenderBlockContent]) * init(style: AsideStyle, name: String, content: [RenderBlockContent]) * init(asideKind: Markdown.Aside.Kind, content: [RenderBlockContent]) Most often DocC will use the fourth initializer to create aside blocks from markdown, via RenderContentCompiler. This initializer, creating an aside from a Swift Markdown Aside.Kind, also contains special logic to determine a capitalized name for the aside that was previously implemented in AsideStyle. Simplify the `RenderBlockContent.AsideStyle` structure to act as if it were an enum of the known styles supported by DocC Render: note, tip, experiment, important, or warning. (Since this is public API, this change retains AsideStyle as a structure with a string raw value.) Also simplify the JSON encoding and decoding of Aside and AsideStyle structures to save and read the style and name in a natural way.
|
@swift-ci please test |
|
@swift-ci please test |
mayaepps
left a comment
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.
Looks good to me, I just left a couple comments.
| /// - name: The name of the aside. | ||
| /// - content: The block content to display in the aside | ||
| /// > Note: | ||
| /// > If the name doesn't match a style supported by DocC Render, set the style to "note". |
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.
Thanks for adding these comments!
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.
minor: is the phrase "set the style to "note"" meant to be an instruction to the caller or an explanation of what the aside does internally with the parameters that the caller passes? It would be good to clarify that.
| let dashReference = ResolvedTopicReference(bundleID: context.inputs.id, path: "/documentation/Asides/dashAsides()", sourceLanguage: .swift) | ||
|
|
||
| // Instead of creating expected aside objects, type out strings here. | ||
| // This makes assertion failures much easier to read and parse. |
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.
I'm not sure I understand why this is - is it because it will be clearer whether the failure came from the aside name vs. style vs. contents? At first glance, it looks like this test is more complicated now to accommodate this.
| } | ||
| return aside | ||
| } | ||
| XCTAssertEqual(25, asides.count) |
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.
Nit: this check looks redundant considering the two assertions below should catch if there are the wrong number of asides.
d-ronnqvist
left a comment
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.
There are 3 categories of changes that I'd ask for:
- The structure of the test that divide up the information into 3 separate tests is really hard to follow. Please update it to structure its test inputs so that it's easier to reason about, easier to debug, and easier to update in the future.
- There's a fair amount of new public initializers and it was hard for me to tell from the diff if they're all tested. Please check that they are and add any tests that missing.
- The documentation comments for the new public initializers confuse me. Please go though them to update any sentence that start with verb like "use" or "set" to clarify who the "actor" is for those sentences; if they are a direction to the caller or a description of what the code does internally.
| ] | ||
|
|
||
| // The aside contents from Tests/SwiftDocCTests/Test Resources/Asides.symbols.json | ||
| let expectedAsideContents = [ |
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.
Please define a private struct or use a tuple for these arrays. It's near impossible to read this ad know which element in the other 2 arrays that for example "This is wrong.", belong to.
Sources/SwiftDocC/Model/Rendering/Content/RenderBlockContent.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/Rendering/Content/RenderBlockContent.swift
Outdated
Show resolved
Hide resolved
| /// - name: The name of the aside. | ||
| /// - content: The block content to display in the aside | ||
| /// > Note: | ||
| /// > If the name doesn't match a style supported by DocC Render, set the style to "note". |
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.
minor: is the phrase "set the style to "note"" meant to be an instruction to the caller or an explanation of what the aside does internally with the parameters that the caller passes? It would be good to clarify that.
Sources/SwiftDocC/Model/Rendering/Content/RenderBlockContent.swift
Outdated
Show resolved
Hide resolved
| style = AsideStyle(displayName: displayName) | ||
| } | ||
| self = try .aside(.init(style: style, content: container.decode([RenderBlockContent].self, forKey: .content))) | ||
| self = try .aside( |
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.
question: do we have any tests that verify the backward compatibility of these decoding changes? In other words; are there any tests that decode aside render block values from a handful of JSON data that was created before this?
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.
There are quite a few new initializers. Does the added tests cover all of them or should we add additional tests so that each initializer is covered?
Sources/SwiftDocC/Model/Rendering/Content/RenderBlockContent.swift
Outdated
Show resolved
Hide resolved
|
|
||
| let asidesSGFURL = Bundle.module.url( | ||
| forResource: "Asides.symbols", withExtension: "json", subdirectory: "Test Resources")! | ||
| let (_, _, context) = try await testBundleAndContext(copying: "LegacyBundle_DoNotUseInNewTests", excludingPaths: []) { url in |
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.
If you're significantly updating this test, please also stop relying on "LegacyBundle_DoNotUseInNewTests"
AsideStyle initializers.
Co-authored-by: David Rönnqvist <ronnqvist@apple.com>
…wift Co-authored-by: David Rönnqvist <ronnqvist@apple.com>
rdar://162549413
Summary
Introduce a new stored property on
RenderBlockContent.Asidecallednameto store the name or title of aside blocks. This allows DocC to independently save and control the styling and the name of asides. Previously DocC conflated the style of the aside block (e.g. colors or other styling) with the name of the aside. Although it isn't currently possible to author an aside with a different style and name, this change allows theAsidemodel itself to support that one day.Aside now has four available initializers:
init(style: AsideStyle, content: [RenderBlockContent])init(name: String, content: [RenderBlockContent])init(style: AsideStyle, name: String, content: [RenderBlockContent])init(asideKind: Markdown.Aside.Kind, content: [RenderBlockContent])Most often DocC will use the fourth initializer to create aside blocks from markdown, via
RenderContentCompiler. This initializer, creating an aside from a Swift MarkdownAside.Kind, also contains special logic to determine a capitalized name for the aside that was previously implemented inAsideStyle.Simplify the
RenderBlockContent.AsideStylestructure to act as if it were an enum of the known styles supported by DocC Render:note,tip,experiment,important, orwarning. (Since this is public API, this change retainsAsideStyleas a structure with a string raw value.)Also simplify the JSON encoding and decoding of the
AsideandAsideStylestructures to save and read the style and name in a natural way.The precise behaviors of each of the four initializers is listed and tested in SemaToRenderNodeTests.swift:
Dependencies
None.
Testing
init(style: AsideStyle, name: String, content: [RenderBlockContent]), serialize the Render JSON and test DocC Render displays the custom aside properly with the specified name and style. (Note this isn't currently possible to write using DocC markdown syntax.)Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded[ ] Updated documentation if necessary