[EDX-146] Make sure every type, method, and attribute in IDL mentions a feature spec point#1477
Merged
[EDX-146] Make sure every type, method, and attribute in IDL mentions a feature spec point#1477
Conversation
lawrence-forooghian
requested review from
Morganamilo,
Peter-Maguire,
QuintinWillison,
Rosalita,
ikbalkaya,
m-hulbert,
maratal,
mohyour,
owenpearson,
stmoreau,
tbedford and
tomkirbygreen
lawrence-forooghian
force-pushed
the
EDX-146-add-spec-points-to-IDL
branch
from
fdc2635 to
dc98d2b
Compare
maratal
reviewed
Jun 30, 2022
maratal
reviewed
Jun 30, 2022
lawrence-forooghian
force-pushed
the
EDX-146-add-spec-points-to-IDL
branch
from
dc98d2b to
bcf7504
Compare
lawrence-forooghian
force-pushed
the
EDX-146-add-spec-points-to-IDL
branch
from
bcf7504 to
785e277
Compare
Peter-Maguire
approved these changes
Jul 5, 2022
maratal
approved these changes
Jul 5, 2022
ikbalkaya
approved these changes
Jul 6, 2022
lmars
reviewed
Jul 6, 2022
This is the only place this property appears in the spec (I’ll be adding an explicit feature spec point shortly). I don’t think it makes any sense for a channel name to be nullable, for at least a couple of reasons: - a channel name is its unique identifier to a user of the SDK - the main way for a user to get hold of a Channel instance is using Channels#get, which takes a non-null name
It’s already in the IDL, and it’s a very sensible thing to have as an attribute on a channel.
This was already in the IDL, and the behaviour described there (proxy to method on underlying Connection) matches the behaviour in a few of our existing SDKs that I looked at: - ably-js@0e4e590 - ably-java@faa89af - ably-ruby@65e0a24 Strangely, ably-cocoa@05f9eb8 does the opposite - all of the connection-related behaviour is implemented in ARTRealtime, and then ARTConnection just proxies method calls through to the corresponding ARTRealtime instance. But I suppose that from a public API point of view the final result is the same.
The behaviour of this overload is already implicitly described by the EventEmitter spec points (RTE2 and RTE5 specifically), but the other overloads are explicitly described in the RealtimeChannel description, so let’s be consistent.
This is similar to 423d85f, except that the feature spec doesn’t state that RealtimePresence implements EventEmitter. I am however not sure _why_ RealtimePresence doesn’t implement EventEmitter, since it seems to offer the same functionality. Also, the no-arg RealtimePresence#subscribe, which is in the IDL, is also present in a bunch of client SDKs I looked at: - ably-js@0e4e590 - ably-java@faa89af - ably-ruby@65e0a24 (here, RealtimePresence actually implements EventEmitter too) - ably-cocoa@05f9eb8
tl;dr: I’m feeling a bit confused and 🤷 about this one. This already existed in the IDL, and is implemented in a few of our client SDKs. I had to guess a bit as to the intended behaviour here. The only existing clue in the feature spec is the IDL’s “proxy for RSA7”, which isn’t that helpful since RSA7 doesn’t describe a method or attribute – it describes a behaviour (that of determining which clientId to use when requesting a token). So perhaps you could argue that it means to say that #clientId should return this value, which is essentially the same as the clientId provided in the ClientOptions. But then you also have RSA12 and RSA17, which describe the behaviour of the Auth#clientId attribute, which is slightly different – essentially, it initially returns the value provided in the ClientOptions, but it can become null if, subsequently, "@ConnectionDetails#clientId@ is @null@ following a connection to Ably". (To be honest, I don’t really understand what are the circumstances under which the ClientOptions would specify a non-null clientId, and then a subsequent connection to Ably would return a null clientId in the ConnectionDetails.) It probably doesn’t help that I still don’t fully understand our auth flow, full stop, so this might need some external input. So, to try and decide which behaviour was the intended one, I looked at some of our client SDKs that implement this attribute: - ably-cocoa@05f9eb8: returns the ClientOptions’s clientId - ably-ruby@65e0a24: proxy to Auth#clientId - ably-dotnet@9a3c31e: proxy to Auth#clientId So, on the very slight balance of numbers, and since I think it makes sense to go for the more dynamic of the two options, I’m opting for the proxy to Auth#clientId. I don’t really understand _why_ we’d want to have this attribute that just proxies an Auth one, given that the Auth object is already exposed publicly on RealtimeClient.
This is the only ClientOptions param that doesn’t have a TO* point. It doesn’t seem intentional.
This is the only AuthOptions param that doesn’t have an AO* point. It doesn’t seem intentional.
RTN14e is the one that describes the retry behaviour when suspended.
The TK2d that’s currently given in the IDL is not the right one, and we didn’t have a correct one.
These attributes are mentioned a lot in the spec, but not explicitly listed.
This type was mentioned, but not explicitly described. I used the batch publish REST API documentation at /rest/batch/#batch-publish to populate this.
Message#extras is, as TM2i says, a JSON object, a container of key value pairs. It doesn’t make sense to talk about it containing a typed DeltaExtras object. I have chosen to keep the DeltaExtras type as part of the public interface, because some of our SDKs (ably-java, ably-ruby, ably-dotnet) provide a mechanism for fetching a typed DeltaExtras object from a message’s extras.
…ons` This is too large to address within the scope of EDX-146 (making sure that every IDL item has a feature spec point against it) and I think can be addressed at another time.
As in 3cb8e2b, except as far as I know this hasn’t yet been implemented in any SDKs to the part about the “mechanism for fetching a typed DeltaExtras object” doesn’t yet apply.
lawrence-forooghian
force-pushed
the
EDX-146-add-spec-points-to-IDL
branch
from
785e277 to
cdd3d0a
Compare
lmars
approved these changes
Jul 12, 2022
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 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 makes sure that every type, method, and attribute in the IDL mentions a feature point spec. It adds feature spec points where necessary, and also checks all of the feature spec points currently mentioned in the IDL for appropriateness, updating the IDL where necessary.
We are doing this as part of our efforts to make the IDL the canonical description of the public interface of our client SDKs. As part of this work, we want to be sure that the IDL is a subset of the information contained in the feature spec points.
Review
Please review commit-by-commit, since there are a lot of changes, and a lot of explanation of what I've done in the commit messages.
Outstanding work
There is no feature spec point describing the inheritance of
ClientOptionsfromAuthOptions– this is a large one and I've split it into a separate task ably/specification#16 to be worked on later.