-
Notifications
You must be signed in to change notification settings - Fork 123
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 default availability inheritance behaviour customizable. #1024
base: main
Are you sure you want to change the base?
Make default availability inheritance behaviour customizable. #1024
Conversation
Added a new info.plist key `CDInheritDefaultAvailability` that allows customization on DocC logic for using default availability as the availability information for symbols. `CDInheritDefaultAvailability` allows two values: - `platformAndVersion` (deafault): This value adds both the platform and the semantic version as a fallback value for symbols that don't define explicit availability information for the given platform. - `platformOnly`: This value only adds the platform name (removes the version) to the symbols that don't define an explicit availability annotation for the given platform. rdar://132980711
Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Workspace/DocumentationBundle+Info.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Workspace/DocumentationBundle+Info.swift
Outdated
Show resolved
Hide resolved
@@ -39,6 +49,10 @@ extension DocumentationBundle { | |||
/// The keys that must be present in an Info.plist file in order for doc compilation to proceed. | |||
static let requiredKeys: Set<CodingKeys> = [.displayName, .identifier] | |||
|
|||
/// The flag to enable or disable symbol availability inference from the module default availability. | |||
/// If not specified the default befaviout will be ``InheritDefaultAvailabilityOptions.platformAndVersion`` | |||
public var inheritDefaultAvailability: InheritDefaultAvailabilityOptions? |
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 can't quite make up my mind about how I would prefer to expose this information in the public API.
Allow me to think out loud for a bit ... 🤔
Currently, this option/configuration only determines whether or not symbols without explicit availability (either from in-source attributes or @Available
directives) will display the versions specified in the Info.plist as the "introduced" version on the render page, or if those symbols will only display the platform name without an introduced version.
The key question to ask ourselves is: "how do we expect this to change in the future?"
If we think that we'll add some other configuration that's independent of this version-behavior, then we would basically need to create a second enum to avoid needing to add enum cases that represent combinations of behaviors:
case platformAndVersion
case platformOnly
case platformAndVersionWithNewBehavior
case platformOnlyWithNewBehavior
In this case, the InheritDefaultAvailabilityOptions
won't extend beyond its original two cases.
If both the new and the old configuration are booleans (or other two-value configuration) then we could represent them using an OptionSet
to allow for arbitrary combinations of them.
On the other hand, if the new configuration isn't a boolean, then we would need to separate it regardless. If the two configuration still are related, then maybe we would want to use a structure to group them.
All that leads me to two mini concussions:
- We possibly won't get much future extensibility from using an enum for this configuration.
- Maybe there's a more specific name we could use for
InheritDefaultAvailabilityOptions
if it only determines this specific version behavior.
🤔
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.
Some of the many possibilities—names are just placeholders—that I can imagine for this are:
A boolean value
var displayInheritedVersionNumbers = true
This would make the current call site
if !bundle.info.displayInheritedVersionNumbers { ... }
If we add more configuration in the future that uses either of the other two possibilities, we could convert this to a computed property that reads from the option set or struct.
We could also use a "negative" boolean, for example var skipInheritedVersionNumbers = false
to avoid the negation at the call site.
An option set
struct DefaultAvailabilityOptions: OptionSet {
let rawValue: Int
static let displayInheritedVersionNumbers = Self(rawValue: 1 << 0)
}
var defaultAvailabilityOptions: DefaultAvailabilityOptions
This would make the current call site
if !bundle.info.defaultAvailabilityOptions.contains(.displayInheritedVersionNumbers) { ... }
If we add more boolean configuration it could be defined as new values in the option set.
New non-boolean configuration would need to separate properties.
A struct with configuration as different properties
struct DefaultAvailabilityOptions {
var displayInheritedVersionNumbers = true
}
var defaultAvailabilityOptions: DefaultAvailabilityOptions
This would make the current call site
if !bundle.info.defaultAvailabilityOptions.displayInheritedVersionNumbers { ... }
If we add more configuration in the future they could be additional members of this struct.
Each of these alternatives (and other) have tradeoffs between now and the future.
If we believe that future changes are not-that-likely then it doesn't matter all that much.
Regardless of what we do now, we can always deprecate it and make it derive its value from the future value. The only case where that doesn't work is if we want to reuse the name but change the type (for example form an enum to a struct).
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.
These vague ideas are a bit hard to talk about in written form like this. Let me know if you would like to about it in person instead.
I have no real concerns with this code. Just hoping for a brief conversation about the public API before it's harder to change.
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.
Same goes for this comment which is about the property list representation of the same configuration.
|
||
var context = try setupContext(inheritDefaultAvailability: """ | ||
<key>CDInheritDefaultAvailability</key> | ||
<string>platformOnly</string> |
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: how likely do you think it is that we'll want to support more values here?
If we want this to behave like a feature flag then we could make it a boolean value in the property list and move the specifics of the what it does into the name. For example, as a "negative" value:
<key>CDDisplayInheritDefaultIntroducedVersions</key>
<false/>
or as a "positive" value:
<key>CDDisplayAvailabilityWithoutInheritVersions</key>
<true/>
(I'm certain that there are better names that are both positive and negative for this)
I'm asking because I have some past experience of naming default values as a behavior that people somehow find and and then people start redundantly adding it because the medium can't communicate that it's already the default and that they get the same behavior by doing nothing.
@@ -59,6 +59,24 @@ struct SymbolGraphLoader { | |||
let bundle = self.bundle | |||
let dataProvider = self.dataProvider | |||
|
|||
/// Computes the default availbiality based on the `inheritDefaultAvailability` option. | |||
let defaultAvailabilities: ([DefaultAvailability.ModuleAvailability]?) -> [DefaultAvailability.ModuleAvailability]? = { defautAvailabilities 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.
Why does this need to be a closure? I can't see a requirement for 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.
Now that you mention it; why is the symbol graph loader responsible for modifying the decoded Info.plist values?
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.
Now that you mention it; why is the symbol graph loader responsible for modifying the decoded Info.plist values?
Is during the symbol graph loading that each symbol gets assigned the default availability data. I could create an intermediary structure to not modify the input data, would that be better? CC: @d-ronnqvist
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.
Unless I'm missing some reason why it wouldn't be possible;
I think it would be better to create the .available(version: nil)
values when decoding the Info.plist so that they don't need to be modified after the fact.
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 think it would be better to create the .available(version: nil) values when decoding the Info.plist so that they don't need to be modified after the fact.
The framework availability information must remain. Modifying the default availability information when decoding it would remove the availability at the top level page.
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 think it would be better to create the .available(version: nil) values when decoding the Info.plist so that they don't need to be modified after the fact.
The framework availability information must remain. Modifying the default availability information when decoding it would remove the availability at the top level page.
To play devil's advocate for a second:
We don't really have a backwards compatibility issue here because the developer needs to specify a new configuration key to opt in to the new behavior.
From one perspective it might be preferable to display the version information on the module page but not the individual symbols' pages, but from another perspective it's more consistent if the configuration applies equally to all pages. At the very least "must" is a much to strong assertion.
If the developer wants to display version information on the module page they can accomplish that by explicitly specifying introduced versions in @Available
directives in the module page's documentation extension 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.
More seriously;
If this is intentionally deferring a modification until after a certain read-access in order to get two different behaviors out of the same property then I don't quite like that.
If we want to different behaviors then I would prefer if we made that behavioral difference explicit.
One way to accomplish that could be to move access behind a function with a parameter. Another way to accomplish that could be to introduce a second property for the other behavior.
After syncing with @d-ronnqvist we've decided that the best direction to move this is to make use of a
|
`DefaultAvailabilityOptions` is a new info plist key that contains a dictionary of options to customise the logic of the default availbaility. The only option for now is `InheritVersionNumber` that lets you add the version into the symbols availability that don't define explicit availability annotation. The default is true.
067ea76
to
59942f6
Compare
…lt availability options.
59942f6
to
4ba25c1
Compare
Bug/issue #, if applicable: 132980711
Summary
Introduced a new Info.plist key,
CDDefaultAvailabilityOptions
, which enables customization of DocC's logic when applying default availability to symbols availability information.CDDefaultAvailabilityOptions
is a dictionary that allows an inner keyInheritVersionNumber
.InheritVersionNumber
is a boolean that allows the removal of the symbol default availability version on symbols that don't define an explicit availability annotation for the given platform.Dependencies
N/A
Testing
Use the attached doc catalog.
Availability.docc.zip
Steps:
Bar
Bar
shows the platforms from the default availability but not the versions.Foo
shows iOS with the version and the rest of the platforms without version.InheritVersionNumber
to true and assert that the availability renders as expected.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded