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`.
* 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.
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.
* 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!
···
on Mon Jun 20 2016, Brent Royal-Gordon <swift-evolution@swift.org> wrote:
--
Dave