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

[Paul Cantrell]

> 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() }

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

____

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.

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.

____

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.

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!

Cheers,

Paul



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20160124/f6ad6133/attachment.html>