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

Channel: consistent publish methods between other platform clients #121

Closed
ricardopereira opened this issue Jan 6, 2016 · 9 comments
Closed
Labels
enhancement New feature or improved functionality.

Comments

@ricardopereira
Copy link
Contributor

Current publish methods

- (void)publish:(art_nullable id)payload callback:(art_nullable ARTErrorCallback)callback;
- (void)publish:(art_nullable id)payload name:(art_nullable NSString *)name callback:(art_nullable ARTErrorCallback)callback;
- (void)publishMessage:(ARTMessage *)message callback:(art_nullable ARTErrorCallback)callback;
- (void)publishMessages:(__GENERIC(NSArray, ARTMessage *) *)messages callback:(art_nullable ARTErrorCallback)callback;

Should be something like

- (void)publish:(art_nullable NSString *)name data:(art_nullable id)data callback:(art_nullable ARTPublishCallback)callback;
- (void)publish:(__GENERIC(NSArray, ARTMessage *) *)messages callback:(art_nullable ARTPublishCallback)callback;

I am assuming renaming ARTErrorCallback to ARTPublishCallback. ARTErrorCallback feels like it is a callback when something fails and it is not.

@ricardopereira ricardopereira added the enhancement New feature or improved functionality. label Jan 6, 2016
@mattheworiordan
Copy link
Member

Agreed, renamed to ARTPublishCallback
I also would prefer to be consistent and use the method publish. However, whilst I did not suggest this approach, I thought that Objective-C recommended not overloading methods and used this approach, is this not the case? @tcard thoughts?

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Well, it is a good practice but with Swift it might be better.

The result with the change:

Objective-C

[channel publish:@"event" data:nil callback:nil];
[channel publish:@[message1, message2] callback:nil];

Swift

channel.publish("event", data: nil, callback: nil)
channel.publish([message1, message2], callback: nil)

@tcard
Copy link
Contributor

tcard commented Jan 6, 2016

I'm not sure what is our "policy" about preferring Ably conventions vs. language conventions. I think we favor the former, right? For example, I've seen that the Go library takes callbacks for async operations, which is really not Go-y. That favors uniformity on documentation I guess, so you can apply doc resources written for one language to one trivially, but it makes it uglier and maybe more difficult to actually use the libraries. So I think I prefer the latter, but what's really important is to be consistent, so if we're doing the former elsewhere, I'm OK with this change.

@tcard
Copy link
Contributor

tcard commented Jan 6, 2016

OK, I've checked and I was wrong about Go taking callbacks. It's also wrong in the front page example, where I saw it:

captura de pantalla 2016-01-06 a les 23 30 29

More reason to stay consistent with language conventions then.

@mattheworiordan
Copy link
Member

I've seen that the Go library takes callbacks for async operations, which is really not Go-y

Yup, we use channels normally.

Our policy is to where sensible keep as consistent as possible with the API, however, we do use idiomatic forms for every language where it feels "wrong" to use our more generic API. So there is no right or wrong answer, however, if overloading in Objective-C is not wrong nor bad, but perhaps just not typical, then I would personally opt for the overloaded approach that @ricardopereira has provided. @tcard or @ricardopereira do you feel overloading like this, for Objective-C developers, would feel wrong, or perhaps just not normal but OK?

@ricardopereira
Copy link
Contributor Author

There are strict conventions to follow for some method names: accessor method names & object constructors method names. But anything else is just fine... Apple just want to sell the expressivity of Objective-C.

By the rules it should be something like:

- (void)publish:(art_nullable id)payload callback:(art_nullable ARTErrorCallback)callback;
- (void)publishWithName:(art_nullable NSString *)name payload(art_nullable id)payload callback:(art_nullable ARTErrorCallback)callback;
- (void)publishWithMessage:(ARTMessage *)message callback:(art_nullable ARTErrorCallback)callback;
- (void)publishWithMessages:(__GENERIC(NSArray, ARTMessage *) *)messages callback:(art_nullable ARTErrorCallback)callback;

So, I would say it is not normal but it is ok. We just need to take in mind to create unique methods within a class. The translation of Objective-C to Swift help us to guarantee that.

@mattheworiordan
Copy link
Member

Ok, well if it's normal but OK I personally would prefer to remain closer to our intended API, thanks @ricardopereira

@tcard
Copy link
Contributor

tcard commented Jan 6, 2016

Yeah, I guess it's not that bad.

@ricardopereira ricardopereira mentioned this issue Feb 2, 2016
tcard added a commit that referenced this issue Feb 15, 2016
Using EventEmitter to emit state changes and messages.

Fixes #134, #183, #121, #189.
@tcard
Copy link
Contributor

tcard commented Feb 15, 2016

Fixed by 97f0b16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improved functionality.
Development

No branches or pull requests

3 participants