Skip to content

Apply fallback availability logic to beta information #910

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

Merged

Conversation

sofiaromorales
Copy link
Contributor

Bug/issue #, if applicable: 127227456

Summary

Apply fallback availability logic to beta information.

For platforms that have a fallback platform, copy the availability information passed through the CLI, including the isBeta value.

The fallback platform logic works correctly for availability information passed through the SGFs but does not apply to availability information passed through the --platform CLI option. This inconsistency caused unexpected behavior. For instance, a symbol would copy the version from iOS, which is also marked as beta, and display it as available for both iOS and iPadOS, but only as beta for iOS.

This PR aims to fix this by also adding the same fallback mechanism to the information about the current release of a platform passed through the CLI.

Dependencies

N/A.

Testing

Steps:

  1. Download the attached catalog.
  2. Run `docc preview --platform name=iOS,version=16.0.0,beta=true
  3. Assert that iPadOS and Catalyst are marked as beta

Availability.docc.zip

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded`
  • Updated documentation if necessary

Comment on lines 171 to 173
DefaultAvailability.fallbackPlatforms.forEach { fallbackPlatform in
if (!currentPlatforms.keys.contains(fallbackPlatform.key.displayName)) {
currentPlatforms[fallbackPlatform.key.displayName] = currentPlatforms[fallbackPlatform.value.rawValue]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why displayName is used on the key but rawValue is used on the value (both are PlatformName elements)?

Also, this can be written simpler using a regular for loop that binds the key and values to separate meaningful names. If you want you can also move the if statement into a where clause on the loop

Suggested change
DefaultAvailability.fallbackPlatforms.forEach { fallbackPlatform in
if (!currentPlatforms.keys.contains(fallbackPlatform.key.displayName)) {
currentPlatforms[fallbackPlatform.key.displayName] = currentPlatforms[fallbackPlatform.value.rawValue]
}
}
for (platform, fallbackPlatform) in DefaultAvailability.fallbackPlatforms where currentPlatforms[platform.displayName] == nil {
currentPlatforms[platform.displayName] = currentPlatforms[fallbackPlatform.rawValue]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarification, there are two pieces to my comment above:

  • A question if there's a potential bug with using .displayName for one of the platforms and .rawValue for the other
  • A non-blocking style suggestions about using a regular loop and using contextually relevant variable names instead of "key" and "value".

Copy link
Contributor Author

@sofiaromorales sofiaromorales May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if the initial choice of using rawValue instead of displayName was intentional, but I don't think it makes a difference since in practice both will be the same (iOS).

However, after reviewing it, I decided to change it to displayName since it has the raw value as fallback and looks more consistent with the rest of the code.

self.displayName = displayName ?? rawValue

I updated the commit with the changes.

For platforms that have a fallback platform, copy the platform availability information passed through the CLI, including the `isBeta` value.

rdar://127227456
@sofiaromorales sofiaromorales force-pushed the 127227456-inherit-beta-tag branch from 69b4dd4 to 14d27a1 Compare May 8, 2024 16:38
Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@patshaughnessy patshaughnessy left a 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

@sofiaromorales
Copy link
Contributor Author

@swift-ci please test

@sofiaromorales sofiaromorales merged commit e824c42 into swiftlang:main May 9, 2024
sofiaromorales added a commit to sofiaromorales/swift-docc that referenced this pull request May 9, 2024
…er. (swiftlang#910)

For platforms that have a fallback platform, copy the platform availability information passed through the CLI, including the `isBeta` value.

rdar://127227456
sofiaromorales added a commit that referenced this pull request May 9, 2024
…er. (#910) (#912)

For platforms that have a fallback platform, copy the platform availability information passed through the CLI, including the `isBeta` value.

rdar://127227456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants