[swift-evolution] Stdlib closure argument labels and parameter names

[Dave Abrahams]

on Tue Jun 21 2016, Dave Abrahams <swift-evolution at swift.org> wrote:

> on Mon Jun 20 2016, Brent Royal-Gordon <swift-evolution at swift.org> wrote:
>
>>> A couple of weeks ago we started to notice that we had some poorly-named
>>> closure parameters and argument labels in the standard library, so we
>>> did a complete audit of the standard library's APIs and came up with a
>>> preliminary proposal for changes, which we applied in a branch and you
>>> can review in https://github.com/apple/swift/pull/2981.  Let's please
>>> carry on further discussion here rather than in the pull request, though.
>>
>> In general, I like this; `orderingBy` is a particularly nice
>> improvement over the old `isOrderedBefore` convention. 
>
> I don't really love the use of “by”, FWIW, but I thought `orderingWith`
> was more confusable (ordering A with B might swap A and B, whereas the
> parameter is a closure).  It could be argued, though, that I am being
> overly concerned with unlikely misinterpretations, at the cost of
> “naturalness”—a known weakness of mine ;-).  Anyway, as ever I'm open to
> discussion on this.
>
>> A few specific comments about things I don't like:
>>
>> * In `map` and `flatMap`, I'm not sure how much `transform` buys us
>>   over `elementTransform`.
>
> I think you mean the converse.  And I agree that `elementTransform`
> is probably not an improvement over `transform`.

...and I've gone back to `transform` in my PR.

>> * In general, I'm not a fan of most of the changes away from `where`
>> labels. 
>
> The only such changes I can find are in
> https://github.com/apple/swift/pull/2981/commits/3418eede88d724ad23731fe8f412f51e03cf5106
>
> Note that part of this change was to make all filter closures
> consistent; in the main `filter` API there was no label at all.
> However, we felt that there's a real clarity problem with the polarity
> of the argument (we talk about “filtering things out” but the closure
> indicates which elements to keep).  And we couldn't find a “where”-based
> name that began to clarify it.
>
> I will argue that even changing to “suchThat,” as in the PR, does not
> sufficiently clarify the closure's polarity, and the only true fix for
> filter is to use a different base name (some have suggested “select,”
> and I have other ideas), but that is out of scope for this particular
> set of changes.  So if the community is happier with a “where” label
> here I can live with it.  I do think “suchThat” is marginally clearer.

I have not received any further pushback on “suchThat,” so I've left it
alone.

>
>> Those are a nice, straightforward convention applied broadly across
>> the Sequence APIs. (Yes, I criticized `where` as a method name in
>> another thread, but I don't think `where` is a problem when there's a
>> function base name to give it context.) When they don't work, that's
>> usually because of a less-than-ideal base name. I'm not saying that
>> *all* base names that aren't compatible with `where` should be
>> changed, but rather that if `where` is not enough, that's an API
>> smell.
>>
>> * In particular, `elementWhere` is not a good label for the same
>> reason that `removeElement` is not a good name. Session 403 last week
>> actually talked about this between roughly minutes 8 and 11. (I'm sure
>> you know about its content; you probably saw it before we did.)
>
> Yes I do, and I think you misinterpreted the message in that session.
> There's nothing wrong with repeating type information when it's
> necessary for clarity or fluency at the use-site.  In the case of
> `contains(elementWhere:)`, it's there for fluency:
>
>        customers.contains(where: isSingle)
>
> doesn't read as well as:
>
>        customers.contains(elementWhere: isSingle)
>
> The point is not to imagine that every argument should be preceded by
> a noun, and repetition of type information is often the result of
> trying to do that.
>
>> * I like `separatedWhere` on `split`, but I think the Equatable
>> version needs a similar renaming. 
>
> That's a nice thought; I think it's arguably out-of-scope here, though.
>
>> Perhaps `separatedBy`?  `separatedOn`? The usual opposite of `where`,
>> `of`, doesn't work here. (Alternatively, `separatedWhere` could be
>> `separatorWhere` instead, but that's not quite as elegant.)
>
> I'd want to consider variations of `separatingAt` or `onSeparator` or
> `atSeparator` too... which makes me thing “separatedWhere” might not be
> as good as “separatingWhere” for the closure version.
>
>> * I'm very uncomfortable with the amount of weight
>> `accumulatingResultBy` adds to `reduce`. `combinedBy` seems perfectly
>> cromulent to me. I'm even more concerned by your suggestion in the
>> pull request body of
>> `accumulating(startingFrom:combiningBy:)`. `reduce` is a subtle and
>> slightly confusing operation; adding more words to its call sites will
>> not solve that problem. If you want to invent a new name from whole
>> cloth, I would probably use something like `combining(with
>> initialResult: T, by nextResult: (T, Element) -> T)`. (For that
>> matter, while we're working in this area, `sequence(first:next:)`
>> could use a similar coat of paint.)
>
> As with `filter(suchThat:`, `reduce(accumulatingResultBy:` is attempting
> to solve with an argument label what IMO is a grave weakness in clarity
> of the base name.  If you read the documentation for `reduce`, you'll
> see that it's all about accumulating a result, and if you consider that
> its current signature often leads to O(N^2) behavior and we are thinking
> about adding an overload that takes its “accumulator” inout, the
> arguments for avoiding the name “accumulate” get progressively weaker.
> But as noted earlier, changing base names is out-of-scope for this
> proposal.  As with “filter,” I could live with leaving this alone,
> though I do believe “accumulatingResultBy:” is a real improvement in
> clarity.

...but I think it's overly specific at the expense of smoothness.  So
I've removed `Result` from that name.

>> * I agree with the comment on GitHub that `invoke` should be
>> `execute`. 
>
> Why?  Rationales help.
>
>> If you see a distinction between the two cases on the number of
>> arguments, I would then suggest `passTo` as the label on these
>> methods: `views.forEach(passTo: addSubview)`,
>> `withUnsafeBufferPointer(&bytes, passTo: Data.init(buffer:))`.
>
> Those are intriguing ideas, but that direction tends to suggest this
> would be better:
>
>   views.passEach(to: addSubview)
>   passUnsafeBufferPointer(to: Data.init(buffer:))
>
> ...until you pass a trailing closure:
>
>   views.passEach { addSubView($0) }
>   passUnsafeBufferPointer { Data.init(buffer:$0) }
>
> (note: withUnsafeBufferPointer takes only one argument, a closure).
>
>>
>> * It's a little odd that you're using `comparingBy` for `Equatable`
>> and `orderingBy` for `Comparable`. Did you judge `equatingBy` to be
>> too awkward? 
>
> Yes, and because it's not “equating,” which would mean using equality
> (==) it's “testing equivalence” with respect to the predicate.
>
>> Perhaps the real problem is that `Equatable` ought to be `Comparable`
>> and `Comparable` ought to be `Orderable`? 
>
> I don't think so, personally, but regardless I consider such a change
> out-of-scope for this proposal.
>
>> Or maybe `comparingBy` should just be something more general, like
>> `matchingBy`? That would make perfectly sensible but slightly odd use
>> cases like this one read better:
>>
>> 	let isAnIdiot = luggageCombination.starts(with: [1, 2, 3, 4,
>> 5], matchingBy: <=) // Matches [1,2,3,4,5], but also [1,1,1,1,1],
>> [1,2,3,2,1], etc.
>
> That would not be legal, as <= is not an equivalence relation.  You
> could think about redefining the meaning of `starts(with:` to not
> require an equivalence relation, but that's something I'm not confident
> *I* know how to do meaningfully, and regardless is again out-of-scope.
>
>> Very soon (hopefully), I will be posting an early draft of a proposal
>> renaming the various first/last/prefix/suffix/etc. APIs. I believe the
>> only place it touches on your proposal is in
>> `starts(with:isEquivalent:)`, but I think your changes to the second
>> parameter label can be easily incorporated into what I'm doing.
>
> Great!

I'm going to write up the proposal ASAP based on the current PR unless I
get more feedback.

Thanks,

-- 
Dave