[ECO-4999] Preparing for use with Xcode 16 #1978
Conversation
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/1978/features
WalkthroughThe changes made in the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1978/jazzydoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Source/ARTDataEncoder.m (2)
187-187: Good improvement on type checking for "vcdiff" encoding.This change enhances type safety by ensuring that the delta codec is only applied to NSData objects. It effectively prevents potential runtime errors and aligns with the PR objective.
For consistency with other parts of the method, consider moving the
isKindOfClasscheck to a separate condition:} else if ([encoding isEqualToString:@"vcdiff"] && _deltaCodec) { if (![data isKindOfClass:[NSData class]]) { errorInfo = [ARTErrorInfo createWithCode:ARTErrorInvalidMessageDataOrEncoding message:@"invalid data type for 'vcdiff' decoding: expected NSData"]; } else { NSError *decodeError; data = [_deltaCodec applyDelta:data deltaId:identifier baseId:_baseId error:&decodeError]; // ... rest of the existing code ... } }This structure would make the error handling more consistent with other encoding cases in this method.
Line range hint
1-245: Consider refactoring for improved maintainability and consistency.While the changes made effectively address the PR objectives, there are some general improvements that could be made to enhance the overall quality of the file:
Method length: The
encode:anddecode:identifier:encoding:methods are quite long. Consider breaking them down into smaller, more focused methods for better readability and maintainability.Error handling consistency: The error handling approach varies slightly between different encoding cases. Consider standardizing the error creation process, perhaps by creating a helper method for generating
ARTErrorInfoobjects with consistent codes and messages.Type checking: The addition of the
isKindOfClass:check for "vcdiff" encoding highlights an opportunity to implement similar checks consistently across all encoding types.Documentation: Adding inline documentation (e.g., using Doxygen-style comments) for the main methods would improve code understandability and maintainability.
These suggestions are not directly related to the current PR objectives but could be considered for future improvements to enhance the overall code quality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Source/ARTDataEncoder.m (2 hunks)
🔇 Additional comments not posted (1)
Source/ARTDataEncoder.m (1)
100-102: Excellent addition of nil check for improved error handling.This change effectively addresses the PR objective by adding a crucial nil check before proceeding with the encoding process. It prevents potential runtime errors and provides a clear error message about the expected input types.
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/1978/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Test/Test Utilities/TestUtilities.swift (2)
1780-1817: LGTM! Consider grouping related extensions.The implementation of CustomStringConvertible for various enums using the @retroactive attribute is correct and consistent. This change improves the string representation of these types when the RetroactiveAttribute feature is available.
Consider grouping related extensions together (e.g., all connection-related enums, all channel-related enums) for better code organization. This can improve readability and maintainability.
Line range hint
1817-1855: LGTM! Consider using a shared implementation.The implementation of CustomStringConvertible for various enums without the @retroactive attribute is correct and consistent. This ensures that the string representation is available even when the RetroactiveAttribute feature is not present.
To reduce code duplication, consider extracting the common implementation into a shared function or macro. This could be achieved using a custom macro or a code generation step, depending on your build process. For example:
#if hasFeature(RetroactiveAttribute) #define CUSTOM_STRING_CONVERTIBLE_EXTENSION(Type, ConversionFunction) \ extension Type: @retroactive CustomStringConvertible { \ public var description: String { \ return ConversionFunction(self) \ } \ } #else #define CUSTOM_STRING_CONVERTIBLE_EXTENSION(Type, ConversionFunction) \ extension Type: CustomStringConvertible { \ public var description: String { \ return ConversionFunction(self) \ } \ } #endif CUSTOM_STRING_CONVERTIBLE_EXTENSION(ARTRealtimeConnectionState, ARTRealtimeConnectionStateToStr) CUSTOM_STRING_CONVERTIBLE_EXTENSION(ARTRealtimeConnectionEvent, ARTRealtimeConnectionEventToStr) // ... repeat for other typesThis approach would centralize the implementation and make it easier to maintain in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Test/Test Utilities/TestUtilities.swift (2 hunks)
- Test/Tests/RealtimeClientChannelsTests.swift (1 hunks)
- Test/Tests/RestClientChannelsTests.swift (1 hunks)
🧰 Additional context used
📓 Learnings (1)
Test/Tests/RestClientChannelsTests.swift (1)
Learnt from: maratal PR: ably/ably-cocoa#1973 File: Test/Tests/RestClientChannelsTests.swift:5-18 Timestamp: 2024-09-17T13:25:42.060Z Learning: Avoid suggesting refactoring the conditional compilation in `RestClientChannelsTests.swift` for `ARTRestChannels` extensions, as the user prefers to keep the current structure.
🔇 Additional comments (3)
Test/Tests/RestClientChannelsTests.swift (1)
5-18: LGTM: Conditional compilation for Xcode 16 compatibilityThe introduced conditional compilation block for the
ARTRestChannelsextension is a good approach to prepare for Xcode 16 compatibility while maintaining backward compatibility. The implementation looks correct and consistent in both cases:
- When
RetroactiveAttributeis available, it uses@retroactive Sequence.- When
RetroactiveAttributeis not available, it uses the standardSequenceprotocol.This change aligns well with the PR objective of preparing the codebase for use with Xcode 16 without switching to it yet.
Test/Tests/RealtimeClientChannelsTests.swift (1)
5-19: Approved: Good preparation for potential Xcode 16 features.The conditional compilation block is a well-thought-out approach to prepare for potential new features in Xcode 16 while maintaining backward compatibility. The use of
@retroactive Sequencewhen theRetroactiveAttributefeature is available shows forward-thinking in terms of adopting new Swift language features.Consider adding a brief comment explaining the purpose of this conditional compilation, such as:
// This conditional compilation block prepares for potential new features in Xcode 16 // while maintaining backward compatibility with older versions.Let's verify if this change impacts other parts of the codebase:
✅ Verification successful
Verified: Conditional compilation change is consistent with the codebase.
The verification confirms that the conditional compilation block in
RealtimeClientChannelsTests.swiftaligns with existing patterns in the codebase. No conflicting implementations or unintended usages ofARTRealtimeChannelsorhasFeature(RetroactiveAttribute)were found, ensuring that the change maintains compatibility and prepares for future Swift features as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of ARTRealtimeChannels and Sequence protocol # Test 1: Search for other implementations or extensions of ARTRealtimeChannels echo "Searching for other ARTRealtimeChannels implementations or extensions:" rg --type swift "class ARTRealtimeChannels|extension ARTRealtimeChannels" -g '!Test/Tests/RealtimeClientChannelsTests.swift' # Test 2: Search for other uses of the Sequence protocol with ARTRealtimeChannels echo "Searching for other uses of Sequence protocol with ARTRealtimeChannels:" rg --type swift "ARTRealtimeChannels.*Sequence" -g '!Test/Tests/RealtimeClientChannelsTests.swift' # Test 3: Search for uses of hasFeature(RetroactiveAttribute) in other files echo "Searching for other uses of hasFeature(RetroactiveAttribute):" rg --type swift "hasFeature\(RetroactiveAttribute\)" -g '!Test/Tests/RealtimeClientChannelsTests.swift'Length of output: 1089
Test/Test Utilities/TestUtilities.swift (1)
Line range hint
1780-1855: Summary: Improved string representation for Ably SDK enumsThese changes successfully add CustomStringConvertible conformance to various enums used in the Ably SDK, improving their string representation. The implementation is correct and uses conditional compilation to handle different feature availability (RetroactiveAttribute).
Key points:
- Improves debugging and logging capabilities for Ably SDK types.
- Maintains compatibility with different feature sets.
- Prepares the codebase for use with Xcode 16.
While the current implementation is functional, consider the suggested refactorings to improve code organization and reduce duplication in future iterations.
github-actions
bot
temporarily deployed
to
staging/pull/1978/jazzydoc
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/1978/features
maratal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.github/workflows/examples.yaml (2)
23-25: LGTM: Updated Xcode selection to version 16.0The change to Xcode 16.0 aligns perfectly with the PR objective. Good job on updating the path and keeping the version output for debugging.
Consider adding a comment explaining why we're preparing for Xcode 16 but not switching to it yet, as mentioned in the PR title. This could help future maintainers understand the context of this change.
Line range hint
1-70: Summary of changes and recommendationsThe changes in this file generally align with the PR objective of preparing for Xcode 16 compatibility. Here's a summary of the main points:
- The macOS and Xcode versions have been appropriately updated.
- There are some inconsistencies with iOS and iPhone versions that need attention:
- The Fastlane scan output directory references iOS 18.0, which doesn't exist yet.
- The build destinations for both Carthage and APNS Example reference iPhone 16, which also doesn't exist.
Please address these issues to ensure the workflow remains functional. Consider using the latest available iOS version and iPhone model, or use generic simulators where appropriate.
To improve maintainability, consider:
- Using variables for version numbers (if supported by your CI system) to make future updates easier.
- Adding comments explaining the rationale behind preparing for Xcode 16 without fully switching to it yet.
These changes will help prepare the project for Xcode 16 while maintaining compatibility with current systems.
.github/workflows/check-pod.yaml (1)
13-13: Suggestion: Update checkout action versionWhile not directly related to the PR objectives, the static analysis tool has flagged that the
actions/checkout@v2action is outdated. Consider updating to the latest version (v4 as of September 2023) for improved features and security:- - uses: actions/checkout@v2 + - uses: actions/checkout@v4This change will ensure you're using the most up-to-date version of the action.
🧰 Tools
🪛 actionlint
13-13: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/integration-test.yaml (1)
Line range hint
1-124: Align changes with PR objectivesThe current changes in this file appear to be moving directly to Xcode 16 and iOS/tvOS 18, which is inconsistent with the PR objective of "Preparing for use with Xcode 16 (but not switch to it yet)". Please review and adjust the changes to ensure they align with the intended goal of preparation without actual migration.
Consider the following steps:
- Keep the current Xcode version (15.1) and iOS/tvOS versions (17.2).
- If necessary, add conditional logic or separate workflows to test compatibility with Xcode 16 without making it the default.
- Ensure all changes are reversible and don't break the current CI pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/check-pod.yaml (1 hunks)
- .github/workflows/docs.yml (2 hunks)
- .github/workflows/examples.yaml (4 hunks)
- .github/workflows/integration-test.yaml (2 hunks)
- fastlane/Fastfile (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/check-pod.yaml
13-13: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (13)
fastlane/Fastfile (2)
Line range hint
1-37: Overall structure and implementation look goodThe
LaneConfigclass and its usage in theLANE_CONFIGSarray provide a clean and extensible way to manage test configurations for different platforms. This approach allows for easy addition or modification of test configurations in the future.The overall structure and implementation of the Fastfile are well-organized and maintainable. Good job on keeping the code modular and easy to understand.
🧰 Tools
🪛 rubocop
[convention] 23-24: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 27-27: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
25-26:⚠️ Potential issueReconsider the iOS and tvOS version updates
The changes to iOS 18.0, tvOS 18.0, and iPhone 16 seem premature, as these versions and devices don't exist yet. This could lead to issues in CI/CD pipelines and local development environments.
Consider the following suggestions:
- Revert to the previous versions (iOS 17.2 and tvOS 17.2) for now.
- Update to the latest stable versions that are compatible with Xcode 16 (likely iOS 17.4 and tvOS 17.4).
- Add a comment explaining the intention to update these versions in the future when iOS 18 and tvOS 18 are released.
Here's a proposed change:
- LaneConfig.new(:test_iOS18_0, "Ably-iOS", ["iPhone 16 (18.0)"]), - LaneConfig.new(:test_tvOS18_0, "Ably-tvOS", ["Apple TV (18.0)"]), + LaneConfig.new(:test_iOS17_4, "Ably-iOS", ["iPhone 15 (17.4)"]), + LaneConfig.new(:test_tvOS17_4, "Ably-tvOS", ["Apple TV (17.4)"]), + # TODO: Update to iOS 18.0 and tvOS 18.0 when they are released and compatible with Xcode 16To ensure we're using the correct versions, let's check the available simulators:
This will help confirm the latest available iOS and tvOS versions for testing.
.github/workflows/docs.yml (2)
13-13: LGTM: Updated runner to macOS-14Updating the runner to
macOS-14is a good step in preparing for Xcode 16 compatibility. This newer macOS version will support the latest Xcode versions and ensure the workflow runs on an up-to-date system.
22-24: Verify if switching to Xcode 16 is intended at this stageThe PR objectives state that we're "Preparing for use with Xcode 16 (but not switch to it yet)". However, this change is actually switching the documentation generation process to use Xcode 16.0.
Is this intentional? If we're not meant to switch to Xcode 16 yet, we might want to keep using the previous version (15.1) for now. Alternatively, if it's safe to use Xcode 16 for documentation generation, we should update the PR description to reflect this change.
Could you please clarify if this switch to Xcode 16 for the documentation workflow is intended at this stage?
.github/workflows/examples.yaml (4)
12-12: LGTM: Upgraded to macOS 14 for Xcode 16 compatibilityThe change to
macos-14is appropriate for preparing the workflow for Xcode 16 compatibility. This aligns well with the PR objective.
54-54:⚠️ Potential issueVerify the iPhone model in the build destination
The build destination has been changed from "iPhone 15" to "iPhone 16". As of now, iPhone 16 doesn't exist. This might cause the build to fail.
Consider using the latest available iPhone model or a more generic simulator. For example:
- xcodebuild build -scheme "AblyCarthage" -destination "platform=iOS Simulator,name=iPhone 16" -configuration "Debug" + xcodebuild build -scheme "AblyCarthage" -destination "platform=iOS Simulator,name=iPhone 15" -configuration "Debug"Or use a generic device:
- xcodebuild build -scheme "AblyCarthage" -destination "platform=iOS Simulator,name=iPhone 16" -configuration "Debug" + xcodebuild build -scheme "AblyCarthage" -destination "generic/platform=iOS Simulator" -configuration "Debug"To check available simulator devices, you can run:
#!/bin/bash xcrun simctl list devicesThis will help you choose an appropriate device for the build destination.
70-70:⚠️ Potential issueVerify the iPhone model in the APNS Example build destination
Similar to the Carthage build, the APNS Example build destination has been changed to "iPhone 16", which doesn't exist yet.
Consider using the latest available iPhone model or a more generic simulator. For example:
- xcodebuild build -scheme "AblyPushExample" -destination "platform=iOS Simulator,name=iPhone 16" -configuration "Debug" + xcodebuild build -scheme "AblyPushExample" -destination "platform=iOS Simulator,name=iPhone 15" -configuration "Debug"Or use a generic device:
- xcodebuild build -scheme "AblyPushExample" -destination "platform=iOS Simulator,name=iPhone 16" -configuration "Debug" + xcodebuild build -scheme "AblyPushExample" -destination "generic/platform=iOS Simulator" -configuration "Debug"To ensure consistency across the workflow, please run:
#!/bin/bash grep -n "iPhone 16" .github/workflows/examples.yamlThis will help identify any other occurrences of "iPhone 16" that may need to be updated.
46-46: Please clarify the iOS version updateThe output directory has been changed from
test_iOS17_2totest_iOS18_0. While updating for newer versions is good, iOS 18 doesn't exist yet. Could you please clarify if this is intentional or if it should be a different version?To check the currently supported iOS versions, you can run:
This will help determine the appropriate iOS version to use.
.github/workflows/check-pod.yaml (2)
11-11: LGTM: Updating to macOS 14 runnerThe change to use
macos-14as the runner is appropriate and aligns with the PR objective of preparing for Xcode 16 compatibility. macOS 14 (Sonoma) is the latest stable version and is required for Xcode 16.
15-19: Verify intention: Switching to Xcode 16The update to use Xcode 16.0 is consistent with the PR objective of preparing for Xcode 16 compatibility. However, the PR title mentions "but not switch to it yet", which seems contradictory to this change. Could you please clarify if the intention is to fully switch to Xcode 16 in this PR or just prepare for it?
If the intention is to fully switch, this change is appropriate. If not, we might need to adjust this step to continue using Xcode 15.1 while ensuring compatibility with Xcode 16.
.github/workflows/integration-test.yaml (3)
43-43:⚠️ Potential issueRevert Xcode path to version 15.1.
Changing the Xcode path to version 16 is premature and inconsistent with the PR's goal of preparing for Xcode 16 without switching to it yet. This change could cause CI failures if Xcode 16 is not available on the runners.
Please revert this change:
- sudo xcode-select -s /Applications/Xcode_16.app + sudo xcode-select -s /Applications/Xcode_15.1.appTo ensure the correct Xcode version is available, run:
#!/bin/bash # Description: Check available Xcode versions on the CI runner # Test: List available Xcode versions ls -l /Applications/Xcode*
24-24:⚠️ Potential issueRevert iOS and tvOS test lanes to current versions.
The changes to
test_iOS18_0andtest_tvOS18_0are inconsistent with the PR objective of not switching to Xcode 16 yet. iOS/tvOS 18.0 are not released and would require Xcode 16, which contradicts the PR's intent.Consider reverting these changes:
- lane: test_iOS18_0 + lane: test_iOS17_2 - lane: test_tvOS18_0 + lane: test_tvOS17_2Additionally, verify that these lanes exist in your Fastfile to prevent CI pipeline failures.
Run the following script to check if the lanes exist in the Fastfile:
Also applies to: 27-27
16-16: Approve the update to macOS 14, but verify compatibility.The change to
macos-14aligns with the PR objective of preparing for newer tools. However, ensure that all required tools and dependencies are compatible with macOS 14 to maintain CI stability.To verify the compatibility, you can run the following script:
maratal
force-pushed
the
fix/1977-xcode16-analyzer-issues
branch
from
458b1d3 to
afe1262
Compare
Closes #1977
Preparing library for use with Xcode 16 (but not exclusevly switching to it yet).
Summary by CodeRabbit
RealtimeClientChannelsandRestChannelsto support retroactive behavior based on feature availability.