Make initialiser for ARTPaginatedResult available #1984
Conversation
WalkthroughThe changes in the Changes
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/1984/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Source/include/Ably/ARTPaginatedResult.h (1)
Line range hint
1-53: Consider adding nullability annotation to the initializer.The code looks good overall, with proper use of nullability annotations and documentation. For consistency with the rest of the interface, consider adding a nullability annotation to the newly public initializer.
Apply this minor change for consistency:
- - (instancetype)init; + - (nullable instancetype)init;This change maintains consistency with the nullability annotations used throughout the rest of the interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Source/include/Ably/ARTPaginatedResult.h (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Source/include/Ably/ARTPaginatedResult.h (1)
Line range hint
1-53: Overall assessment: Changes align with PR objectives and maintain code quality.The modification to make the initializer public achieves the goal of enabling mocking and subclassing of ARTPaginatedResult. The existing code structure, documentation, and use of nullability annotations demonstrate good coding practices. The suggested improvements are minor and focus on maintaining consistency within the interface.
Remember to update any relevant documentation or guides to reflect this new capability of directly initializing ARTPaginatedResult instances.
github-actions
bot
temporarily deployed
to
staging/pull/1984/jazzydoc
…d subclassing in other SDKs e.g. Chat
umair-ably
force-pushed
the
allowARTPaginatedResultMocking
branch
from
76bc268 to
7148957
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Source/ARTPaginatedResult.m (1)
140-143: Approved: Good addition to prevent misuse of the class.The new initializer method effectively prevents the creation of
ARTPaginatedResultinstances without the required parameters. This aligns well with the PR objective and enforces proper usage of the class.A minor suggestion for improvement:
Consider adding a more descriptive error message to guide developers on how to properly initialize the class. For example:
- [NSException raise:NSInternalInconsistencyException format:@"ARTPaginatedResult can't be initialized with init."]; + [NSException raise:NSInternalInconsistencyException format:@"ARTPaginatedResult can't be initialized with init. Use initWithItems:rest:relFirst:relCurrent:relNext:responseProcessor:logger: instead."];This change would provide clearer guidance to developers who might accidentally use the wrong initializer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Source/ARTPaginatedResult.m (1 hunks)
- Source/include/Ably/ARTPaginatedResult.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/include/Ably/ARTPaginatedResult.h
🧰 Additional context used
🔇 Additional comments (1)
Source/ARTPaginatedResult.m (1)
140-143: Verify impact and update documentationThe addition of this initializer enforces proper usage of the
ARTPaginatedResultclass, which is a positive change. However, to ensure a smooth transition:
- Verify that this change doesn't break existing code in other parts of the codebase or dependent SDKs.
- Update the class documentation to clearly state the correct way to initialize
ARTPaginatedResult.Run the following script to check for potential impacts:
Consider updating the class documentation in the header file (
ARTPaginatedResult.h) to clearly state the correct initialization method. This will help prevent future misuse and provide clear guidance for developers.
There was a problem hiding this comment.
Make initialiser for ARTPaginatedResult available - allows mocking and subclassing in other SDKs e.g. Chat
I don't really understand what this PR does or why; please could you clarify?
- What does the "available" in "Make initialiser for ARTPaginatedResult available" mean? You still get a runtime error if you try to call it.
- If you wanted to subclass it in another SDK, what's the designated initializer that the subclass should call?
| @@ -137,4 +137,9 @@ + (void)executePaginated:(ARTRestInternal *)rest withRequest:(NSMutableURLReques | |||
| }]; | |||
| } | |||
|
|
|||
| - (nonnull instancetype)init { | |||
| [NSException raise:NSInternalInconsistencyException format:@"ARTPaginatedResult can't be initialized with init."]; | |||
| __builtin_unreachable(); | |||
There was a problem hiding this comment.
What does __builtin_unreachable do? I've never seen it before.
Make initialiser for ARTPaginatedResult available - allows mocking and subclassing in other SDKs e.g. Chat
Summary by CodeRabbit
ARTPaginatedResultclass, allowing users to create instances directly with the required parameters.ARTPaginatedResultclass by enforcing a specific initialization pattern, ensuring proper instance creation.