-
Notifications
You must be signed in to change notification settings - Fork 131
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
Make NavigatorIndex.Builder
ignore language variants when requested
#1077
Make NavigatorIndex.Builder
ignore language variants when requested
#1077
Conversation
Clients might want the Navigator Index to not preemptively add an entry for nodes that have a language variant trait. For example, a client might not have a renderer cable of applying language variants, in this instance it doesn't make sense to generate a navigator hierarchy for other languages. rdar://138183564
7ba0ed3
to
103bb5b
Compare
@swift-ci please test |
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.
LGTM
@@ -613,12 +613,13 @@ extension NavigatorIndex { | |||
|
|||
/// Index a single render `RenderNode`. | |||
/// - Parameter renderNode: The render node to be indexed. | |||
public func index(renderNode: RenderNode) throws { | |||
/// - Parameter ignoringLanguage: Whether language variants should be ignored when indexing this render node. | |||
public func index(renderNode: RenderNode, ignoringLanguage: Bool = false) throws { |
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.
Introducing this parameter remains source stable because it has a default value, so exiting calls to .index(renderNode: renderNode)
.
However, if we later wanted to change the name of this parameter, that would be source breaking.
Considering that, are we happy with ignoringLanguage
or do we want to include the word "variants" somewhere in this parameter name?
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.
Since both the parameter documentation and the code comment above where this value is checked mentions variants, I'm pretty sure that future us will understand what it's for regardless.
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.
What do you think? I don't have a strong opinion.
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 also fine either way.
Bug/issue #, if applicable: rdar://138183564
Summary
Clients might want the Navigator Index to not preemptively add an entry for nodes that have a language variant trait. For example, a client might not have a renderer cable of applying language variants, in this instance it doesn't make sense to generate a navigator hierarchy for other languages.
Testing
Updated unit tests to validate the behavior
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded