-
Notifications
You must be signed in to change notification settings - Fork 220
don't automatically transfer the child elements' ranges to the parent #202
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
don't automatically transfer the child elements' ranges to the parent #202
Conversation
@swift-ci Please test |
} | ||
|
||
/// Default implementation of `init(_:inheritSourceRange:)` that discards the `inheritSourceRange` parameter. | ||
public init<Children: Sequence>(_ children: Children, inheritSourceRange: Bool) where Children.Element == BlockMarkup { |
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.
FYI: you should be able to express the same generic constraint with (unless there's something about the protocol conformance that doesn't allow that)
public init<Children: Sequence>(_ children: Children, inheritSourceRange: Bool) where Children.Element == BlockMarkup { | |
public init(_ children: some Sequence<BlockMarkup>, inheritSourceRange: Bool) { |
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.
Weren't some
and any
added in a relatively recent Swift release? What's our minimum supported Swift version for Swift-Markdown?
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 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 just ran a test using a Swift 5.7 Docker image and it seemed to build just fine, so i went ahead and added 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.
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.
Right, the reason this code was written like this was because of the way the original protocol requirement was written. If we want to do this conversion in one place we should do it everywhere. 😅
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.
And now that i'm looking through the rest of the codebase, the same treatment can be applied to BasicInlineContainer
, the initializers on the Doxygen elements, and more places that were written before some Protocol
was widespread...
@swift-ci Please test |
92eab26
to
b9a54e9
Compare
@swift-ci Please test |
@swift-ci Please test |
Bug/issue #, if applicable: rdar://136218537
Summary
The
subsitutingChild(_:through:)
method on RawMarkup introduced in #196 has the option to preserve the element's original source range. However, this fell back to the new child's range if its parent had no source range. This caused an issue where the assertion in the Aside initializer was being tripped when a BlockQuote had no parsed range, but its inner Paragraph did. This PR removes the fallback and just copies in the parent's source range if thepreserveRange
option is set.In a separate commit, i also added a new initializer requirement to
BasicBlockContainer
that adds aninheritSourceRange
option, to allow the new container to inherit the source range of its children even though it's a synthetic wrapper. This method can be used in Swift-DocC when a list item is converted into an aside to propagate that source range up through the new aside.Dependencies
None
Testing
As this is an API change, the functionality is covered by automated testing.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded