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

[Dave Abrahams]

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

>> On Jan 24, 2016, at 2:23 PM, Paul Cantrell via swift-evolution
>> <swift-evolution at swift.org> wrote:
>> 
>> 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.
> …
>> 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>…
>> 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.
>
> Here is that separate message as promised, with real-world examples of
> where the guidelines didn’t seem right for Siesta for reasons other
> than first arg naming.
>
> Interestingly, all of them are part-of-speech questions. I’m
> intentionally writing up these notes before perusing Erica’s grammar
> writeup, so I can capture the raw reactions before letting them be
> colored by that document. Erica, I’ll get to your doc soon!
>
> _____________
>
> Siesta uses the widespread fluent style of attaching callbacks to a
> request:
>
>     resource.load()
>         .onCompletion { _ in stopSpinnyThing() }
>         .onSuccess { _ in showHappyFace() }
>         .onFailure { _ in showSadFace() }
>         .onNewData { _ in soundTrumpets() }
>
> These methods mutate the receiving request (by adding a callback), and
> the guidelines thus say that they should be named as imperative
> verbs. The results of that don’t seem like an improvement:
>
>     resource.load()
>         .addCompletionCallback { _ in stopSpinnyThing() }
>         .addSuccessCallback { _ in showHappyFace() }
>         .addFailureCallback { _ in showSadFace() }
>         .addNewDataCallback { _ in soundTrumpets() }

Agreed; the ".onXXX" paradigm is part of an EDSL, and you often can't
follow conventional rules for naming without breaking the power of an
EDSL.  What you have here is code that is notionally declarative,
(i.e. non-mutating) that just happens to do some mutation.

You could reformulate this in various ways so that it was technically in
conformance, e.g...

     resource.load().addHandlers(
         .onCompletion { _ in stopSpinnyThing() },
         .onSuccess { _ in showHappyFace() },
         .onFailure { _ in showSadFace() },
         .onNewData { _ in soundTrumpets() },
     )

...though I don't think you should necessarily feel compelled by the
guidelines to make a change like this.

> In a related case, it’s possible to attach a callback to be run before
> any request within a given context:
>
>     $0.config.beforeStartingRequest {
>         _ in performDrumroll()
>     }
>
> It’s not even clear to me what the imperative verb here should
> be. Maybe “callBeforeStartingRequest”? The guideline-conforming
> options are all awkward; the guideline-breaking form above is clear.
>
> I’d suggest a rule along these lines (but with less convoluted
> phrasing, if anyone can figure that out):
>
> “When a method’s last argument is a closure, the method’s name and
> other arguments may read as a clause which modifies the closure as if
> it were a sentence with an imperative verb.”
>
> Um, yeah, mouthful. Examples may help:
>
> 	Before starting a request, ← method
> 	perform a drumroll. ← closure
>
> 	On completion, ← method
> 	stop the spinny thing. ← closure

I think you're pointing at a sub-category of EDSLs here.  I definitely
don't want to try to cover a subset of EDSLs in the guidelines and I am
almost as sure that I don't want to touch EDSLs at all.  The design of
declarative embedded languages is an art, and those who are going to
engage in it need to have the courage of their convictions :-).

> ____
>
> The ResourceObserver protocol, which clients will implement
> frequently, consists of “response to event” methods that may or may
> not mutate the receiver, but probably have side effects. These are
> methods clients will implement, but probably not call. An example:
>
>     func resourceChanged(resource: Resource, event: ResourceEvent) {
>         tableView.reloadData()
>     }
>
> The name “resourceChanged” feels right to me. The old-school Cocoa
> name would be resourceDidChange, which also feels OK. Options that are
> strictly nouns or verbs — resourceChange, respondToResourceChange —
> all feel awkward by comparison.

Agreed.

> I think this has something to do with the “responding to event” nature
> of the method, crossed with the fact that it’s named in a protocol. We
> can’t know what it does, so we it doesn’t make sense to describe it as
> a verb. We only know why it’s called, what situation it’s responding
> to.
> ____
>
> On the other side of the verb/noun coin, the ResponseTransformer
> protocol consists of a single method…
>
>     public protocol ResponseTransformer {
>         @warn_unused_result
>         func process(response: Response) -> Response
>     }
>
> …which is purely functional — should mutate neither the receiver nor
> the argument — yet just really, really seems wrong to me if it’s not
> named “process.” Why? I don’t have a rule. My gut just tells me that
> method should be a verb.

Yep, it's not returning an altered version of the receiver; it's
returning an altered version of its argument.

> ____
>
> Last up, the rule that all enum cases should be nouns didn’t seem to
> fit here:
>
>     public enum ResourceEvent: CustomStringConvertible {
>         case ObserverAdded
>         case Requested
>         case RequestCancelled
>         case NewData(NewDataSource)
>         case NotModified
>         case Error
>     }
>
> Noun alternatives such as “ObserverAddition” and “RequestStart”
> somehow come with more mental friction.

Agreed again.

> Again, I don’t have a rule to propose here — and in these cases, we
> may just have to say “this is why guidelines are guidelines, not
> laws,” and leave it at that. Or someone can propose better names,
> which would be awesome!

The names look really well-chosen to me, but we'll have to see what 
others think.

-- 
-Dave