-
Notifications
You must be signed in to change notification settings - Fork 7
[ECO-5577] Pre-v1 public API audit #358
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
Merged
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
1416be6
Fix access level of an initializer
lawrence-forooghian 6930dda
Remove unnecessary `@unchecked`
lawrence-forooghian ce15be3
Remove PaginatedResult's conformance to Equatable
lawrence-forooghian 753cb6b
Mark PaginatedResult as @MainActor
lawrence-forooghian bc9de70
Add note allowing us to add ErrorCode values
lawrence-forooghian 07cda47
Fix capitalisation of "clientID" in public API
lawrence-forooghian e85d229
Update comment
lawrence-forooghian 10914f5
Change some struct properties from let to var for consistency
lawrence-forooghian 928c4c3
Fix access level of an initializer
lawrence-forooghian 81c267b
Add some missing public initializers
lawrence-forooghian bffe235
Remove dodgy MessageVersion Equatable implementation
lawrence-forooghian 693b5ea
Remove Equatable from RoomStatusChange and DiscontinuityEvent
lawrence-forooghian 4e3c55b
Add missing room status case helpers
lawrence-forooghian 44b3bd1
Remove Identifiable conformance from Message
lawrence-forooghian 5a99d1e
Remove Equatable from ChatMessageEvent
lawrence-forooghian cdf97f3
Remove Equatable conformance from all the options types
lawrence-forooghian e60ad5c
Remove public String-valued event type enums
lawrence-forooghian 464bf10
Make message reactions send / delete signatures clearer
lawrence-forooghian f8a37a5
Remove `reactions` from Message.copy
lawrence-forooghian b2d84bf
Clarify documentation of Message.copy
lawrence-forooghian 09fa18c
Use fatalError for fetching occupancy data when not switched on
lawrence-forooghian a334284
Fix copy-and-paste error
lawrence-forooghian 52fe739
Remove parameter label from Message.with
lawrence-forooghian 9fc668d
Fix "undefined" in copy of JS documentation
lawrence-forooghian File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
But it's not "sending message serial", it's sending a reaction to the message with message serial. Ditto for
delete.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 don't think that the existing signature makes it clear what the argument means, though. It's just a
Stringwithout any additional information.I agree that you're not "sending a message serial", the same way that in:
Messages.send(params: SendMessageParams)you're sending a message, not "sending a params"Presence.enter(data: PresenceData)you're not "entering a data"etc.
And I'm not 100% sure what's the right signature for these things in Swift. Like, would the following be better, or just more verbose? I don't know.
MessageReactions.send(forMessageWithSerial:)Messages.send(withParams:)Presence.enter(withData:)etc
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 key part of the API design guidelines though is "Prefer method and function names that make use sites form grammatical English phrases."
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.
So maybe the latter are better?
Uh oh!
There was an error while loading. Please reload this 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 so, more descriptive and reflects things correctly. Feel free to resolve this once you decide the names.
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.
OK — can I do that one in a separate PR just so I don't have to deal with merge conflicts on #378?
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.
Gonna merge 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.
Also consider this:
MessageReactions.send(for messageWithSerial:)@lawrence-forooghian but I'm not sure, leaving you to decide.