[swift-evolution] [Review] SE-0023 API Design Guidelines

[Dave Abrahams]

on Sun Jan 24 2016, Paul Cantrell <swift-evolution at swift.org> wrote:

> I really like the general spirit of these guidelines. I particularly
> appreciate their emphasis on clarity at the point of use, and their
> recognition that both brevity and verbosity can be the enemy of
> clarity.
>
> Some of the particulars of the guidelines haven’t worked well for me
> in practice — and I see from this thread that others have hit some of
> the same problems. The guidelines as they stand feel like they’ve
> mostly been vetted against a bunch of ADTs (which is probably true —
> stdlib?), and could use exposure to a broader range of libraries.

They've been vetted against all of Cocoa (to the best of our
ability—it's huge) and against a couple of sample projects, Lister and
DemoBots.  This was a major effort and we endeavored to cover all the
bases.  However, we recognize that there's a lot more code out there
than we can possibly look at, so we're very happy to get more input from
the community.

> Immediately after the guidelines were published, before this review
> began, I tried applying them to Siesta. The project has 244 public
> declarations, of which I found 28 that the guidelines seemed to
> recommend changing. Of those 28, after much reflection and discussion,
> I ended up changing only 7 to match the guidelines (and also cleaning
> up several others, but in a non-guideline-compliant way).
>
> In short, in a real-world project, pre-guidelines code agreed with the
> guidelines 89% of the time, but where it disagreed, the guidelines
> achieved only a 25% acceptance rate with the curmudgeonly developers.
>
> You can follow that discussion here:
>
> 	https://gist.github.com/pcantrell/22a6564ca7d22789315b
> <https://gist.github.com/pcantrell/22a6564ca7d22789315b>
> 	https://github.com/bustoutsolutions/siesta/issues/15
> <https://github.com/bustoutsolutions/siesta/issues/15>
>
> Several places where we rejected the guidelines relate to the raging
> debate about first argument labels.  Here’s a rundown of those
> particular cases. (Since this message will be a long one, I’ll share
> in a separate message some notes on the other places where we rejected
> the guidelines on questions other than the first arg label.)
>
> Hopefully some more concrete examples can be useful in informing the
> discussion.
>
> _____________________________
>
> Quick context: there are things called resources. Resources can have
> observers. Observers are either self-owned or have an external owner
> object. Observers can either conform to a protocol or be closures; if
> the the latter, then they _must_ have an external owner (because
> closures aren’t objects).
>
> There are thus three different methods to add observers to a resource
> — though they all cover the same underlying notion of “observer” (and
> in fact all boil down to the same thing internally):
>
>     resource.addObserver(foo)
>
>     resource.addObserver(fancyIndicator, owner: foo)
>
>     resource.addObserver(owner: foo) {
>         resource, event in
>         updateStuff(resource.latestData)
>     }

Oh, that's an interesting API.  I think it's pretty unusual that the
direct object of the method is a trailing closure, and IMO inventing
some conventions for handling this kind of API in your own code would be
perfectly appropriate.

> The API guidelines as stated would have us change that last one to:
>
>     resource.addObserverWithOwner(foo) {
>         resource, event in
>         updateStuff(resource.latestData)
>     }
>
> However, this incorrectly implies that there is a distinct kind of
> thing that is an “observer with owner,” and that we will get one only
> from the third flavor of the method. That implication is wrong.

Yes, one of the complaints I have about the guidelines trying so hard to
avoid initial argument labels is that it causes some things to be read
as closely-associated when they should not be. The guidelines prevent us
from using that opening parenthesis to express a lack of association.

This comes up especially with Bool parameters, e.g.

  dismissAnimated(false)

where "animated" is essentially a parameter tuning the behavior of the
basic functionality, "dismiss," rather than an intrinsic part of that
functionality.

Others developing the API guidelines felt that the parenthesis was not
significant enough to be meaningful to people reading the call, and that
any increased expressivity was not paid for by the fact that the surface
of our APIs would be less-consistent (a more even balance of APIs with
and without initial labels is less consistent than having initial labels
be quite rare).

> The consistency in the original of the “addObserver” name and the
> “owner:” label make the correct implication: all three methods serve
> the same purpose, observers are observers, and “owner” means the same
> thing in the two places it appears. I certainly think the
> non-compliant version of the code reads better.

+1

> There was extensive discussion around another family of methods that
> return a resource given either a path fragment or an absolute
> URL. This is where we ended up:
>
>     service.resource("/foo")
>     service.resource(absoluteURL: "http://bar.com")
>     service.resource(absoluteURL: NSURL(string: "http://bar.com"))
>
> (The first is by far the most common, the “standard” flavor.)

Sorry, perhaps it should be obvious, but why do you need the label at
all here?  Seems to me you could do this with two overloads, since it's
easy to detect when a string is an absolute URL.

> The guidelines would have had us do this:
>
>     service.resourceWithPathFragment("/foo")
>     service.resourceWithAbsoluteURL("http://bar.com")
>     service.resourceWithAbsoluteURL(NSURL(string: "http://bar.com"))
>
> To my eyes, this crosses the line into verbosity that impedes clarity,

+1

> but there’s an even more serious problem: it wrongly implies that
> there’s a distinction between “resources with path fragments” and
> “resources with absolute URLs.” That’s dangerously wrong. One of the
> central conceits of the whole library is that _all_ resources get a
> canonicalized absolute URL, and there’s a uniqueness guarantee for
> that URL no matter whether it was constructed for a path fragment, an
> absolute URL, or resource-relative navigation.

Yep, that's the "can't use ( to disassociate" problem again.

> In the cases of both addObserver(…) and service.resource(…), the
> guidelines would have us actively mislead users.
>
> Another trickier example of the same issue is the much-discussed
> typedContent method, which downcasts based on inferred type and
> returns a default value if content is either missing or of the wrong
> type:
>
>     var image: UIImage?
>     ...
>     image = imageResource.typedContent(ifNone: placeholderImage)
>
> How to make this conform to the guidelines? The obvious fix is
> terrible:
>
>     image = imageResource.typedContentIfNone(placeholderImage)
>
> This implies … what? That the method only returns typed content if
> there is none of the placeholder image? 

Very similar to dismissAnimated.

> No, clearly a rephrasing is necessary:
>
>     image = imageResource.typedContentWithDefault(placeholderImage)
>
> But again we have this problem of determining whether “with default”
> describes the method’s behavior, its result, or its first
> argument. Are we attaching content with the default somehow attached
> to it? (No.) Are we returning some content, or a default value if
> there is none? (Yes.) So maybe this is better:
>
>     image = imageResource.typedContentOrDefault(placeholderImage)
>
> But now my brain is having parsing problems. What is the LHS of that
> “or?” It just doesn’t read naturally. OK, maybe even more words can
> save us:
>
>     image =
> imageResource.typedContentOrDefaultIfNone(placeholderImage)
>
> Yuck. At this point, we might as well stuff the entire method abstract
> in the name:
>
>     image =
> imageResource.typedContentOrDefaultIfNoneOrMismatchedType(placeholderImage)
>
> Yuck squared. The original is so much clearer:
>
>     image = imageResource.typedContent(ifNone: placeholderImage)

Point taken, but if you let go of determining the return type by type
inference (pretty close to overloading on return type) you might be
better off.

      i = Image(foundIn: resource, default: placeholder)

> IMO, there’s nothing wrong with leaning on programming language syntax
> to help segment and clarify English syntax.

+1.

> _______________________________
>
> What’s the better guideline on first argument labels?
>
> Radek had a nice thought in the aforementioned Gihub thread:
>
>> The rationale being, ifNone doesn't really describe the method … it
>> describes the parameter. Most of the time, the job of the method
>> makes the first parameter obvious (hence the guideline), but here, it
>> doesn't. So the parameter makes sense.

That echoes many of my thoughts.

> I’ll give a +1 for these two recommendations from Erica, which run
> along the same lines as Radek’s thought, but in more thorough detail:
>
>> On Jan 23, 2016, at 6:33 PM, Erica Sadun via swift-evolution
> <swift-evolution at swift.org> wrote:
>> 
>> Prefer external names for the first parameter when the natural
>> semantic relationship between the parameters is stronger than their
>> relation to the operation. 
>> 
>> For example, the following calls use labels for the first parameter:
>> 
>> login(userName: "blah", password: "...")
>> moveTo(x: 50.0, y: 30.0)
>> 
>> This example is contrary to Swift's normal naming scheme which
>> integrates the first argument into the function or method name, for
>> example:
>> 
>> loginWithUserName("blah", password: "...")
>> moveToX(50.0, y: 30.0)
>> 
>> The coupling between x and y, username and password, (and yes it is a
>> judgement call) should be considered as a reason to employ an
>> external label.
> …
>> 
>> Differentiate related calls whose implementations are distinguished
>> by their parameters, as you would with initializers, using first
>> parameter labels.
>> 
>> Instead of loginWithUserName("blah", password: "...") and
>> loginWithCredential(myCredential),
>> prefer:
>> 
>> login(userName: "blah", password: "...")
>> login(credential: myCredential)
>
> I’m not sure we’ve found the perfect, crisp way of saying all this —
> but I strongly agree that the existing guidelines are too rigid on the
> question of the first arg label, and Erica’s wording comes the closest
> I’ve seen to being a viable replacement.

I have a few problems with the above wording, but I'm all for loosening
the restriction.  Unfortunately, getting the wording right isn't the big
challenge; the challenge is convincing the people that matter that the
increased expressivity of allowing first argument labels in more places
is worth the increased non-uniformity of APIs.  Having examples like the
ones you've presented here should be very helpful.

Thanks,

-- 
-Dave