[swift-evolution] Stdlib closure argument labels and parameter names
brent at architechies.com
Mon Jun 20 18:52:51 CDT 2016
> 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. 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`.
* In general, I'm not a fan of most of the changes away from `where` labels. 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.)
* I like `separatedWhere` on `split`, but I think the Equatable version needs a similar renaming. 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'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.)
* I agree with the comment on GitHub that `invoke` should be `execute`. 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:))`.
* It's a little odd that you're using `comparingBy` for `Equatable` and `orderingBy` for `Comparable`. Did you judge `equatingBy` to be too awkward? Perhaps the real problem is that `Equatable` ought to be `Comparable` and `Comparable` ought to be `Orderable`? 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.
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.
More information about the swift-evolution