General adjustment to API spec. #204

tcard · 2016-02-09T15:15:53Z

This pull request is a rather big chain of fixes to adjust to the API spec.

Because most commits rely on some changes from previous commits in the chain, it's not possible to separate them in independent pull requests, so rather than have a series of pull requests each waiting on the previous one to be merged I'm just pushing them all together. It's probably easier to review each commit than the whole thing at once.

Sorry I didn't do this more gradually, but I often edited old commits as I discovered things to fix in an old commit's concern.

ricardopereira · 2016-02-09T18:47:55Z

README.md


-## Manual Installation 
+You can install Ably for iOS either thorugh CocoaPods or manually.


Typo: thorugh

Fixed at 493e0bf.

mattheworiordan · 2016-02-09T19:42:11Z

@tcard wow, this is a MEGA PR :)
I am quite sad to see how far from the API the client library was, but am impressed at how much you've fixed in this PR. A big 👍

ricardopereira · 2016-02-10T00:09:22Z

README.md

+**Objective-C**
+
+```objective-c
+[ARTTestUtil testRealtime:options callback:^(ARTRealtime *client) {


Should this example use ARTTestUtil?

Oops, no. I copypasted more than appropriate from the test. Fixed at 493e0bf.

ricardopereira · 2016-02-10T08:10:26Z

ably-ios/ARTRealtimeChannel.m

@@ -424,23 +404,22 @@ - (ARTErrorInfo *)attach {
    [self.realtime send:attachMessage cb:nil];


I think you need to pass the cb, right?

Right, good catch. Fixed at 0e36e0b.

ricardopereira · 2016-02-10T08:50:52Z

👏🏻👏🏻👏🏻 Great work.
I contributed in some of the API inconsistency and I'm sorry.
Luckily, I'm learning a lot with you guys. Ty.

tcard · 2016-02-12T15:28:44Z

OK, I finally got all tests passing in my machine. Let's see what the CI says, but it should be mergeable now after your LGTM. @ricardopereira @mattheworiordan

tcard · 2016-02-14T14:00:11Z

There are some tests that pass when ran individually but fail when ran in bulk. Although this is introducing this bug, I think we can merge this since it is blocking all other work and we can deal with that afterwards.

mattheworiordan · 2016-02-14T14:26:47Z

There are some tests that pass when ran individually but fail when ran in bulk. Although this is introducing this bug, I think we can merge this since it is blocking all other work and we can deal with that afterward

Agreed, failures like this are test suite issues, not necessarily issues with the library. Perhaps merge and add an issue

ricardopereira · 2016-02-15T09:49:28Z

@tcard I removed this block of code: https://github.com/ably/ably-ios/blob/5d85a840cd31d1c4d687b7bd848a86628695d18b/ablySpec/RealtimeClientConnection.swift#L469-L473. Tests are passing now https://travis-ci.org/ably/ably-ios/builds/109306828. Maybe it is something wrong with dispose.

Why didn't you use Quick? https://github.com/ably/ably-ios/blob/5d85a840cd31d1c4d687b7bd848a86628695d18b/ablySpec/ReadmeExamples.swift#L15
If you want to run only one test, it will always run all the examples first and then the test that you selected.

This is not good for debugging: https://github.com/ably/ably-ios/blob/5d85a840cd31d1c4d687b7bd848a86628695d18b/ably-ios/ARTConnection.m#L79
. E.g.: I can't step into - (__GENERIC(ARTEventListener, ItemType) *)on:(void (^)(ItemType __art_nullable))cb.

#204 (comment)

tcard · 2016-02-15T12:18:30Z

Rebased without changes.

tcard · 2016-02-15T12:45:06Z

Sorry, scratch that, there were some tests added to master that now don't compile. Fixing.

ricardopereira · 2016-02-15T12:54:33Z

Well, that's a problem about how badly Quick works; we should be able to run tests individually. I didn't need all the spec boilerplate (describe/context/it) for these tests, just to run some snippets. There's no use case to describe here.

@tcard You right. Quick generates each test dynamically. That's why it sucks when you try to execute one test with the Xcode shortcut.

Hmm, why not? You should be able to. I've tried and you go directly to ARTEventEmitter on, you can't look into ARTConnection on but that's just a wrapper that calls ARTEventEmitter on.

Ok, if you say so. I didn't pay to much attention to it but it felt like it wasn't stepping into the ARTEventEmitter.

Sorry, scratch that, there were some tests added to master that now don't compile. Fixing.

Can you try clean all the warnings too?

tcard · 2016-02-15T13:10:19Z

Can you try clean all the warnings too?

Done that as well. PTAL

ricardopereira · 2016-02-15T14:25:40Z

Great, thank you.
LGTM

General adjustment to API spec.

tcard force-pushed the adjust-api-spec branch 3 times, most recently from a0e3f92 to 29b80f6 Compare February 9, 2016 16:09

ricardopereira reviewed Feb 9, 2016
View reviewed changes

ricardopereira reviewed Feb 10, 2016
View reviewed changes

tcard force-pushed the adjust-api-spec branch from 29b80f6 to 493e0bf Compare February 10, 2016 00:38

ricardopereira reviewed Feb 10, 2016
View reviewed changes

ricardopereira mentioned this pull request Feb 10, 2016

RTL6i #194

Merged

tcard force-pushed the adjust-api-spec branch from fb040ca to 0e36e0b Compare February 10, 2016 16:07

ricardopereira mentioned this pull request Feb 12, 2016

RTL7d #209

Merged

tcard mentioned this pull request Feb 12, 2016

RTL7c #208

Merged

ricardopereira mentioned this pull request Feb 12, 2016

RTL7b #207

Merged

tcard mentioned this pull request Feb 14, 2016

Release POD #10

Closed

tcard and others added 16 commits February 15, 2016 13:17

Remove logger from constructors, add to ClientOptions.

94b60ef

Add Connection proxies to Realtime public interface.

ce84aa6

Re-arrange option fields to adapt to new spec.

6e2e3e8

#204 (comment)

Add RestChannel.publish(name, data, clientId) method.

5775a09

RealtimeChannel: add 0-ary unsubscribe and off methods.

258f146

Use EventEmitter for RealtimePresence.subscribe.

dd1909d

Message: swap constructor arguments to adjust to spec.

b7a6b4c

Fix: wrong state used in RealtimeAttachTest.

3742784

Fix: bring back needed nil checks.

fea1d58

#204 (comment)

Fix: was still using old ARTSubscription when emitting Presence.

d69258a

Fix: add missing returns after calling error callbacks in attach.

8a1349a

Fix: missing fields in ClientOptions.copyWithZone and isBasicAuth.

5c4acfa

Fix a few tests broken after API refactor.

7b5f8b2

Fix: options' hosts and port weren't properly copied.

60bd0ae

Tests: add missing client.dispose() and close().

b695bcb

Tests: removed client reuse

25a95de

tcard force-pushed the adjust-api-spec branch from faa4385 to 25a95de Compare February 15, 2016 12:17

Fix: adapt new tests to new API after rebase.

b88b833

Fix a few compiler warnings.

f28902e

ricardopereira mentioned this pull request Feb 15, 2016

Carthage support #214

Merged

ricardopereira added a commit that referenced this pull request Feb 15, 2016

Merge pull request #204 from ably/adjust-api-spec

e68836c

General adjustment to API spec.

ricardopereira merged commit e68836c into master Feb 15, 2016

tcard mentioned this pull request Feb 15, 2016

RealtimeChannel: attach is inconsistent with the API #189

Closed

ricardopereira deleted the adjust-api-spec branch February 19, 2016 08:36

tcard mentioned this pull request Feb 19, 2016

Readme examples updated + Swift support #105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General adjustment to API spec. #204

General adjustment to API spec. #204

tcard commented Feb 9, 2016

ricardopereira Feb 9, 2016

tcard Feb 10, 2016

mattheworiordan commented Feb 9, 2016

ricardopereira Feb 10, 2016

tcard Feb 10, 2016

ricardopereira Feb 10, 2016

tcard Feb 10, 2016

ricardopereira commented Feb 10, 2016

tcard commented Feb 12, 2016

tcard commented Feb 14, 2016

mattheworiordan commented Feb 14, 2016

ricardopereira commented Feb 15, 2016

tcard commented Feb 15, 2016

tcard commented Feb 15, 2016

ricardopereira commented Feb 15, 2016

tcard commented Feb 15, 2016

ricardopereira commented Feb 15, 2016


		## Manual Installation
		You can install Ably for iOS either thorugh CocoaPods or manually.

		@@ -424,23 +404,22 @@ - (ARTErrorInfo *)attach {
		[self.realtime send:attachMessage cb:nil];

General adjustment to API spec. #204

General adjustment to API spec. #204

Conversation

tcard commented Feb 9, 2016

ricardopereira Feb 9, 2016

Choose a reason for hiding this comment

tcard Feb 10, 2016

Choose a reason for hiding this comment

mattheworiordan commented Feb 9, 2016

ricardopereira Feb 10, 2016

Choose a reason for hiding this comment

tcard Feb 10, 2016

Choose a reason for hiding this comment

ricardopereira Feb 10, 2016

Choose a reason for hiding this comment

tcard Feb 10, 2016

Choose a reason for hiding this comment

ricardopereira commented Feb 10, 2016

tcard commented Feb 12, 2016

tcard commented Feb 14, 2016

mattheworiordan commented Feb 14, 2016

ricardopereira commented Feb 15, 2016

tcard commented Feb 15, 2016

tcard commented Feb 15, 2016

ricardopereira commented Feb 15, 2016

tcard commented Feb 15, 2016

ricardopereira commented Feb 15, 2016