Uh oh!
There was an error while loading. Please reload this page.
This repository was archived by the owner on Aug 10, 2026. It is now read-only.
[EDX-146] Address some differences between the IDL and the feature spec - #1462
Merged
tbedford merged 15 commits intoJun 27, 2022
Merged
Conversation
lawrence-forooghian
marked this pull request as ready for review
June 15, 2022 12:51
lawrence-forooghian
requested review from
QuintinWillison, SimonWoolf, lmars, m-hulbert, owenpearson, paddybyers and tbedfordJune 15, 2022 12:51
lawrence-forooghian added a commit
that referenced
this pull request
Jun 15, 2022
lawrence-forooghian
requested review from
Morganamilo, Peter-Maguire, Rosalita, ikbalkaya, maratal, mohyour, stmoreau and tomkirbygreen
and removed request for
SimonWoolf, lmars, m-hulbert, paddybyers and tbedfordJune 15, 2022 17:15
Peter-Maguire
suggested changes
Jun 16, 2022
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lawrence-forooghianforce-pushed
the
EDX-146-address-some-IDL-differences-with-feature-spec
branch
from
June 16, 2022 19:40
5908a92 to
aaeeeb5CompareQuintinWillison
approved these changes
Jun 17, 2022
maratal
reviewed
Jun 17, 2022
Uh oh!
There was an error while loading. Please reload this page.
maratal
approved these changes
Jun 17, 2022
lawrence-forooghianforce-pushed
the
EDX-146-address-some-IDL-differences-with-feature-spec
branch
from
June 19, 2022 18:29
aaeeeb5 to
14f7526CompareIt was missing.
It’s more consistent with the rest of the spec and also the IDL.
It was missing.
lawrence-forooghianforce-pushed
the
EDX-146-address-some-IDL-differences-with-feature-spec
branch
from
June 20, 2022 13:36
14f7526 to
17b377eCompareIn RTE5, “no arguments” is being used to mean “not even a listener” - i.e. it treats a listener as an argument, which I think makes sense and is consistent with the way listeners are represented in the IDL. So make the rest of the spec consistent with this.
The spec implies that objects that implement the EventEmitter interface are responsible for emitting their own events - i.e. there’s no reason why a user of the library would want to tell an object to emit an event.
It mentions “optionally a JSON-encodable object”. It’s not clear what this means – either it means that you can pass a JSON-encodable object _in addition to_ or _instead of_ the DeviceDetails argument. Either way, I can’t find anything to back this up. The IDL just allows a DeviceDetails object, as do a few of our SDKs that I checked: - ably-java@7fc1939 - ably-ruby@f5eac15 - ably-cocoa@aed6c60 - ably-js@48a66d6
RSC1a describes a _single_ String constructor that’s responsible for deciding if the passed value is a a key or token.
As specified by RSC19a.
These variants that take a DeviceDetails argument are not mentioned in RSH1b1 or RSB1b4, and from a quick look at a few client SDKs it doesn’t appear to be implemented in our SDKs either.
I think it’s better for the IDL to be complete than to avoid a little duplication.
It’s still part of the public API of the SDK. (Also, for consistency, since our other deprecated properties are in the IDL.)
As mentioned in RTN13a.
lawrence-forooghianforce-pushed
the
EDX-146-address-some-IDL-differences-with-feature-spec
branch
from
June 20, 2022 13:42
17b377e to
3e542b6ComparePeter-Maguire
approved these changes
Jun 27, 2022
tbedford
deleted the
EDX-146-address-some-IDL-differences-with-feature-spec
branch
June 27, 2022 10:58
7 tasks
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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.
Description
This addresses some differences between the IDL and the feature spec - sometimes by changing the IDL, sometimes by changing the feature spec. Please review commit-by-commit – the commit messages contain plenty of details.