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

[Xiaodi Wu]

I'll duly oblige with some pushback on `suchThat`. I get that you're trying
to clarify whether filter retains or gets rid of elements that match the
predicate, but I don't think "filter such that" expresses this idea at all.

Comparing to "filter where," "filter such that" is equally susceptible to
misinterpretation that you are filtering to remove elements that are
matched. For example: "find me some apples, filtering such that are
bruised."

I'd suggest that if you want to be perfectly clear, you'd need something
like `filter(keepingWhere:)`.
On Wed, Jun 22, 2016 at 18:33 Dave Abrahams via swift-evolution <
swift-evolution at swift.org> wrote:

>
> 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
>
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20160622/bf21e7cb/attachment.html>