Skip to content
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

Require agents specified by ClientOptions#agents to have a version #112

Closed

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Nov 14, 2022

This comes from a conversation I had with Quintin about trying to figure out the best way to represent “this agent has no version” in Objective-C, where a dictionary cannot contain a nil value.

He suggested that it would be best to simply remove this scenario by requiring a version, since the #agents property is only for use by other Ably SDKs (as already made explicit by RSC7d6b), which will always have a version.

This comes from a conversation I had with Quintin [1] about trying to figure
out the best way to represent “this agent has no version” in Objective-C, where
a dictionary cannot contain a nil value.

He suggested that it would be best to simply remove this scenario by requiring
a version, since the #agents property is only for use by other Ably SDKs (as
already made explicit by RSC7d6b), which will always have a version.

[1] ably/ably-cocoa#1525 (comment)
@lawrence-forooghian lawrence-forooghian changed the title Require agents specified by ClientOptions#agents to have a version Require agents specified by ClientOptions#agents to have a version Nov 14, 2022
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review November 14, 2022 12:17
lawrence-forooghian added a commit to ably/ably-cocoa that referenced this pull request Nov 14, 2022
This removes the non-spec -[ARTClientOptions addAgent:version:] and
replaces it with a dictionary -agents property, hence implementing
RSC7d6 as described by the spec.

This includes the clarification re nullability added as RSC7d6c in the
currently-open pull request ably/specification#112.

Closes #1525.
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

It doesn't logically follow from RSC7d6b that the Ably authored SDK won't ever have a requirement to append an unversioned agent. I think your point in the PR description assumes that the SDKs using this option will only ever use it to represent themselves which I don't think is a safe assumption (even if it's probably true for all SDKs currently).

If objective-c doesn't allow dictionaries with nil values is there not something like the rust Option type that we can use instead?

@lawrence-forooghian
Copy link
Collaborator Author

I think your point in the PR description assumes that the SDKs using this option will only ever use it to represent themselves which I don't think is a safe assumption

That's a very good point.

If objective-c doesn't allow dictionaries with nil values is there not something like…

Yep, there are definitely ways that we can get around it, I just went with the spec change because it was what Quintin was leaning towards, but I think given your point above it makes more sense for me to address this at the ably-cocoa level. Gonna put this back into draft and remove reviewers whilst I think about it. Thanks for your input!

@lawrence-forooghian
Copy link
Collaborator Author

Closing this one.

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

Successfully merging this pull request may close these issues.

2 participants