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

Implicit impl of controller ifaces #807

Merged
merged 2 commits into from Apr 11, 2017

Conversation

Projects
None yet
6 participants
@kingces95
Member

kingces95 commented Mar 9, 2017

Description of Change

Convert from explicit to implicit implementation + [EditorBrowsable(EditorBrowsableState.Never)] of controller interfaces. Either way the members of the controller iface are hidden from customers however the latter has the advantage that it does not hide the members from the XF team (we use a ProjectReference to reference core and in that case the attribute is ignored by isense) which allows us to simplify all our call sites from ((ICtrlIface)Foo).Bar to Foo.Bar. I've update the callsites for only one controller interface in this PR. If/when consensus has been reached on this change of convention then I'll merge this PR and create a second PR that updates the remaining callsites.

Bugs Fixed

None

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@dnfclas dnfclas added the cla-required label Mar 9, 2017

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Mar 9, 2017

Member

I'm leaning on the other way. Implements all internals interfaces explicitly.

Our platform code is, or will be, used as reference for other implementation and we should consume the exact same API

Member

StephaneDelcroix commented Mar 9, 2017

I'm leaning on the other way. Implements all internals interfaces explicitly.

Our platform code is, or will be, used as reference for other implementation and we should consume the exact same API

@kingces95

This comment has been minimized.

Show comment
Hide comment
@kingces95

kingces95 Mar 9, 2017

Member

I'd argue that we still consume the same the API as our customers. The difference is in how the API is consumed. We consume the core API directly (which, IMHO, results in more readable code) while our customers consume the API through an interface -- but in either case we're consuming the same API.

On the other hand, your point is well taken that the implementation of the renderer and custom renderer do not look the same even if they are consuming the same API. It just that IMHO the benefit of allowing customers to copy/paste from our renderers doesn't outweigh the cost of the less readable callsites.

@jassmith, it's your show, let me know if you'd like me to proceed simplifying the callsites or to hold off.

Member

kingces95 commented Mar 9, 2017

I'd argue that we still consume the same the API as our customers. The difference is in how the API is consumed. We consume the core API directly (which, IMHO, results in more readable code) while our customers consume the API through an interface -- but in either case we're consuming the same API.

On the other hand, your point is well taken that the implementation of the renderer and custom renderer do not look the same even if they are consuming the same API. It just that IMHO the benefit of allowing customers to copy/paste from our renderers doesn't outweigh the cost of the less readable callsites.

@jassmith, it's your show, let me know if you'd like me to proceed simplifying the callsites or to hold off.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Apr 11, 2017

Member

Can you rebase @kingces95 , @jassmith should we pull this?! think if so we need to do it now or it will be harder to always being rebasing.

Member

rmarinho commented Apr 11, 2017

Can you rebase @kingces95 , @jassmith should we pull this?! think if so we need to do it now or it will be harder to always being rebasing.

@rmarinho rmarinho merged commit 093a4ef into master Apr 11, 2017

2 of 6 checks passed

Android-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1
Details
iOS10-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10
Details
iOS8-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8
Details
iOS9-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3764, ignored: 10
Details

@rmarinho rmarinho deleted the implicitctrliface branch Apr 11, 2017

@samhouts samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment