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

Add delegate method naming recommendation #181

Closed
wants to merge 1 commit into from

Conversation

fabienwarniez
Copy link

No description provided.

@fabienwarniez fabienwarniez force-pushed the master branch 2 times, most recently from 14e62f3 to 7074816 Compare May 6, 2016 05:02
@fabienwarniez
Copy link
Author

My recommendation is a mix of the 2 ways that Apple use. The UIScrollViewDelegate and UITableViewDelegate do things differently when there are more than 1 parameter.

@fabienwarniez
Copy link
Author

@JRG-Developer @rayfix Hey guys, any concerns / opinion on this PR?

@jawwad
Copy link
Contributor

jawwad commented May 12, 2016

I'll just throw my opinion in here. I think there are pros and cons to including this. In one way it just specifies to "do it the way that Apple does it" and it just explains the convention that Apple uses. On the other hand it explains things for readers that may not be completely familiar with Apple's conventions, but it makes the style guide longer at the expense of explaining these things to beginners. But let's see how @rayfix feels about it.

@fabienwarniez
Copy link
Author

I would say that we should include it because Apple's recommendation is not clear to everyone, and because they don't stick to their own standards. Compare table view delegate and scroll view delegate, 2 different ways of doing the same thing. We should formulate one and stick to it.

@rayfix
Copy link
Contributor

rayfix commented May 14, 2016

Thanks for pushing this along. I appreciate the effort.

A few thoughts.

There is an issue of timing. The next update is scheduled for late Fall corresponding to the release of Swift 3. The plan was to only fix outright errors or things that the team leads needed to be changed immediately. In any case, new features are not merged to master but to an update branch. The update branch is then reviewed by the RW team before it is moved to master and becomes the "law." :]
[Aside: My github-fu isn't the greatest, and I don't know if it is possible for me to to change the pull request to go to swift3 instead of master. I might have to ask you to issue a new pull request against the swift3 branch.]

I am also getting beaten up a bit about the length of the guide. It think it is useful to add some guidance here, but I was envisioning it could be a fairly small section. I also think that deferring to Apple, can be confusing since their style has evolved. UIScrollView has been around since 2.0 so it is probably more interesting to look at UICollectionView or perhaps a new API that arrives this June! Also, the guide should not be too UIKit focused. These are just my thoughts ... open to feedback.

Thanks again. 😄

@AverageHelper
Copy link

I personally enjoy the legibility and consistent conciseness throughout the language (especially since Swift 3). In writing delegate methods though, I'm having trouble deciding exactly how to write them "Swiftily," as it were. Do I go with Apple's Cocoa Guidelines for Objective-C, as has heretofore been the thing, or do I figure some other way than parameter names to express the overall intent of the method?

Documentation comments are useful, but I don't like sacrificing clarity or legibility for the sake of repetitive type information and overlong method calls.

Unless of course I'm just really bad at writing delegate protocols... fooScene(_ scene: FooScene, inout willFinishMovingToPosition position: FooPosition, forPlayer player: PlayerNode) is a really long way to say, "hey, I'm gonna stop here, unless you want me to stop somewhere else."

Couldn't I say something like
positionWillChange(inout to position: FooPosition, for player: PlayerNode, in scene: FooScene), or
fooScene(_ scene: FooScene, inout willFinishMovingTo position: FooPosition, for player: PlayerNode)

Each of these has its downsides, such as loss of clarity of origin (is this a delegate method from a FooScene, or something I declared locally?) and dangling prepositions (willFinishMoving_To_:). Are these legitimate concerns? Am I just dumb? Are these even the right concerns to be having here? I mean, it's not like it isn't super easy to refactor these later, right?? (thanks Xcode refactorer...)

Or should I just go for it anyway... It is only internal implementation after all...

@doozMen
Copy link

doozMen commented Aug 3, 2016

I would like this to be included. We have discussions with new developers about this and no clear point to point them too. Would be grate if it is in this style guide.

@rayfix
Copy link
Contributor

rayfix commented Oct 26, 2016

Sorry, would it be possible for you to re-target this PR against the swift3 branch sometime this week? (Apologies if there is a way for me to do this.)

@rayfix
Copy link
Contributor

rayfix commented Nov 24, 2016

this pull request is against the wrong branch

@rayfix rayfix closed this Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants