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

Rearrange types relationships to avoid ambiguities. #376

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Conversation

tcard
Copy link
Contributor

@tcard tcard commented Apr 8, 2016

Previously, we had some situations in which, for convenience, we were
inheriting rather than composing (RealtimeChannel inheriting from
RestChannel). This was causing problems in places where some parent
class implemented method:(A)a while a child class was implementing
method:(B)b. Swift 2.2 doesn't let you do this, so it's necessary for
that, but it's also a good idea for Swift <2.2 if only for code
organization and soundness.

Also added some changes in the test Swift code for forwards
compatibility.

@tcard tcard force-pushed the rearrange-types branch 3 times, most recently from 7fefeb0 to 124c2be Compare April 8, 2016 02:25
@ricardopereira
Copy link
Contributor

Great 👌🏻 LGTM

Previously, we had some situations in which, for convenience, we were
inheriting rather than composing (RealtimeChannel inheriting from
RestChannel). This was causing problems in places where some parent
class implemented `method:(A)a` while a child class was implementing
`method:(B)b`. Swift 2.2 doesn't let you do this, so it's necessary for
that, but it's also a good idea for Swift <2.2 if only for code
organization and soundness.

Also added some changes in the test Swift code for forwards
compatibility.
@tcard tcard force-pushed the rearrange-types branch from 124c2be to 624c866 Compare April 8, 2016 09:45
@tcard tcard merged commit f8a1f86 into master Apr 8, 2016
@ricardopereira ricardopereira deleted the rearrange-types branch April 8, 2016 18:33
@mattheworiordan
Copy link
Member

LGTM. Can you confirm this does not impact on our documentation?

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.

3 participants