Make default availability inheritance behaviour customizable. #1024
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,16 @@ | |
| import Foundation | ||
|
|
||
| extension DocumentationBundle { | ||
|
|
||
| /// Options to define the inherit default availability behaviour. | ||
| public enum InheritDefaultAvailabilityOptions: String, Codable { | ||
| /// The platforms with the designated versions defined in the default availability will be used by the symbols as availability information. | ||
| /// This is the default behaviour. | ||
| case platformAndVersion | ||
| /// Only the platforms defined in the default availability will be passed to the symbols. | ||
| case platformOnly | ||
| } | ||
|
|
||
| /// Information about a documentation bundle that's unrelated to its documentation content. | ||
| /// | ||
| /// This information is meant to be decoded from the bundle's Info.plist file. | ||
|
|
@@ -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. 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 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 platformOnlyWithNewBehaviorIn this case, the If both the new and the old configuration are booleans (or other two-value configuration) then we could represent them using an 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:
🤔 There was a problem hiding this comment. Some of the many possibilities—names are just placeholders—that I can imagine for this are: A boolean valuevar displayInheritedVersionNumbers = trueThis 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 An option setstruct DefaultAvailabilityOptions: OptionSet {
let rawValue: Int
static let displayInheritedVersionNumbers = Self(rawValue: 1 << 0)
}
var defaultAvailabilityOptions: DefaultAvailabilityOptionsThis 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 propertiesstruct DefaultAvailabilityOptions {
var displayInheritedVersionNumbers = true
}
var defaultAvailabilityOptions: DefaultAvailabilityOptionsThis 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. 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. Same goes for this comment which is about the property list representation of the same configuration. |
||
|
|
||
| enum CodingKeys: String, CodingKey, CaseIterable { | ||
| case displayName = "CFBundleDisplayName" | ||
| case identifier = "CFBundleIdentifier" | ||
|
|
@@ -47,6 +61,7 @@ extension DocumentationBundle { | |
| case defaultAvailability = "CDAppleDefaultAvailability" | ||
| case defaultModuleKind = "CDDefaultModuleKind" | ||
| case featureFlags = "CDExperimentalFeatureFlags" | ||
| case inheritDefaultAvailability = "CDInheritDefaultAvailability" | ||
|
|
||
| var argumentName: String? { | ||
| switch self { | ||
|
|
@@ -60,6 +75,8 @@ extension DocumentationBundle { | |
| return "--default-code-listing-language" | ||
| case .defaultModuleKind: | ||
| return "--fallback-default-module-kind" | ||
| case .inheritDefaultAvailability: | ||
| return nil | ||
| case .defaultAvailability, .featureFlags: | ||
| return nil | ||
| } | ||
|
|
@@ -91,20 +108,23 @@ extension DocumentationBundle { | |
| /// - defaultCodeListingLanguage: The default language identifier for code listings in the bundle. | ||
| /// - defaultAvailability: The default availability for the various modules in the bundle. | ||
| /// - defaultModuleKind: The default kind for the various modules in the bundle. | ||
| /// - inheritDefaultAvailability: The option to enable or disable symbol availability inheritance from the module default availability. | ||
| public init( | ||
| displayName: String, | ||
| identifier: String, | ||
| version: String?, | ||
| defaultCodeListingLanguage: String?, | ||
| defaultAvailability: DefaultAvailability?, | ||
| defaultModuleKind: String?, | ||
| inheritDefaultAvailability: InheritDefaultAvailabilityOptions? | ||
| ) { | ||
| self.displayName = displayName | ||
| self.identifier = identifier | ||
| self.version = version | ||
| self.defaultCodeListingLanguage = defaultCodeListingLanguage | ||
| self.defaultAvailability = defaultAvailability | ||
| self.defaultModuleKind = defaultModuleKind | ||
| self.inheritDefaultAvailability = inheritDefaultAvailability | ||
| } | ||
|
|
||
| /// Creates documentation bundle information from the given Info.plist data, falling back to the values | ||
|
|
@@ -236,6 +256,10 @@ extension DocumentationBundle { | |
| self.defaultModuleKind = try decodeOrFallbackIfPresent(String.self, with: .defaultModuleKind) | ||
| self.defaultAvailability = try decodeOrFallbackIfPresent(DefaultAvailability.self, with: .defaultAvailability) | ||
| self.featureFlags = try decodeOrFallbackIfPresent(BundleFeatureFlags.self, with: .featureFlags) | ||
| let inheritDefaultAvailabilityRawValue = try decodeOrFallbackIfPresent(String.self, with: .inheritDefaultAvailability) | ||
| if let inheritDefaultAvailabilityRawValue { | ||
| self.inheritDefaultAvailability = InheritDefaultAvailabilityOptions(rawValue: inheritDefaultAvailabilityRawValue) | ||
| } | ||
| } | ||
|
|
||
| init( | ||
|
|
@@ -245,7 +269,8 @@ extension DocumentationBundle { | |
| defaultCodeListingLanguage: String? = nil, | ||
| defaultModuleKind: String? = nil, | ||
| defaultAvailability: DefaultAvailability? = nil, | ||
| featureFlags: BundleFeatureFlags? = nil, | ||
| inheritDefaultAvailability: InheritDefaultAvailabilityOptions? = nil | ||
| ) { | ||
| self.displayName = displayName | ||
| self.identifier = identifier | ||
|
|
@@ -254,6 +279,7 @@ extension DocumentationBundle { | |
| self.defaultModuleKind = defaultModuleKind | ||
| self.defaultAvailability = defaultAvailability | ||
| self.featureFlags = featureFlags | ||
| self.inheritDefaultAvailability = inheritDefaultAvailability | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -272,14 +298,16 @@ extension BundleDiscoveryOptions { | |
| /// - fallbackDefaultModuleKind: A fallback default module kind for the bundle. | ||
| /// - fallbackDefaultAvailability: A fallback default availability for the bundle. | ||
| /// - additionalSymbolGraphFiles: Additional symbol graph files to augment any discovered bundles. | ||
| /// - inheritDefaultAvailability: Option to configure default availability inheritance behaviour. | ||
| public init( | ||
| fallbackDisplayName: String? = nil, | ||
| fallbackIdentifier: String? = nil, | ||
| fallbackVersion: String? = nil, | ||
| fallbackDefaultCodeListingLanguage: String? = nil, | ||
| fallbackDefaultModuleKind: String? = nil, | ||
| fallbackDefaultAvailability: DefaultAvailability? = nil, | ||
| additionalSymbolGraphFiles: [URL] = [], | ||
| inheritDefaultAvailability: DocumentationBundle.InheritDefaultAvailabilityOptions? = nil | ||
| ) { | ||
| // Iterate over all possible coding keys with a switch | ||
| // to build up the dictionary of fallback options. | ||
|
|
@@ -304,6 +332,8 @@ extension BundleDiscoveryOptions { | |
| value = fallbackDefaultModuleKind | ||
| case .featureFlags: | ||
| value = nil | ||
| case .inheritDefaultAvailability: | ||
| value = inheritDefaultAvailability | ||
| } | ||
|
|
||
| guard let unwrappedValue = value else { | ||
|
|
||
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.
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.
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.
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
@Availabledirectives 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.