-
Notifications
You must be signed in to change notification settings - Fork 158
Remove redundant "bundle" parameters where a context is also available #1309
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
Remove redundant "bundle" parameters where a context is also available #1309
Conversation
…er.referencesForSymbols
@swift-ci please test |
9283b45
to
1cfcdd0
Compare
@swift-ci please test |
…onContext methods
1cfcdd0
to
299719a
Compare
@swift-ci please test |
556de91
to
83fe89f
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.
Looks good to me.
Question: Is there a longer term plan to remove DocumentationBundle entirely? Here you remove passing the bundle into many call sites, but we still have other places that reference DocumentationBundle itself.
let documentationContext = self.documentationContext | ||
|
||
let renderContentFor: (ResolvedTopicReference) -> RenderReferenceStore.TopicContent = { reference in | ||
let renderContentFor: (ResolvedTopicReference) -> RenderReferenceStore.TopicContent = { [renderer, documentationContext] reference 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.
This is new Swift syntax for me - does this destructure renderer and documentationContext from the reference value passed into the closure? Why isn't this written the other way around? Like this:
... = { reference [renderer, documentationContext] 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.
The variables within angle brackets is a Swift closure is a "capture list".
The same way that the previous code was shadowing renderer
and documentationContext
with local variables to avoid strongly capturing self
in the closure, the capture list captures renderer
and documentationContext
explicitly as their own variables instead of them referring to self.renderer
and self.documentationContext
which would capture self
.
let (_, _, context) = try await testBundleAndContext(copying: "LegacyBundle_DoNotUseInNewTests", excludingPaths: []) { url in | ||
try? FileManager.default.copyItem(at: asidesSGFURL, to: url.appendingPathComponent("Asides.symbols.json")) | ||
} | ||
defer { |
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 isn't this needed any more? testBundleAndContext no longer creates an actual bundle/file?
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.
It hasn't been needed since somewhere 2021/2022 because testBundleAndContext(copying:...)
already clears up any temporary files that it creates.
includeTaskGroups: Bool = true | ||
) -> [LinkDestinationSummary] { | ||
let bundle = context.bundle | ||
let bundle = context.inputs |
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.
let bundle = context.inputs | |
let inputs = context.inputs |
Why not rename the local variable to match? And throughout the PR I see you retain the identifier bundle
aren't we trying to move away from using bundle?
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 did most of the renaming using IDE renaming actions which doesn't catch locally scoped variables. I can remove this one variable since I need to rebase the PR anyway.
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 found another similarly named variable and updated both in e1c04b7
@swift-ci please test |
Not to remove it completely but to rename it to better reflect the information that it represents (a collection of input files for the context). There is already a |
Bug/issue #, if applicable:
Summary
Now that DocumentationContext is known have exactly one inputs/bundle, types and methods don't need both a context parameter and a inputs/bundle parameter.
This PR goes through various types one-by-one and removes the bundle parameters and updates all the callers to stop passing it.
The main benefit of this is that it's no longer possible to pass a bundle/inputs that don't belong to the context.
Another outcome of this is that many tests who call the context loading test helpers now ignore the bundle. A follow up PR, after this is merged, will clean up the test helpers and have the caller access the bundle/inputs from the context where needed.
Dependencies
None.
Testing
Nothing in particular. This isn't a user-facing change.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded[ ] Updated documentation if necessary