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

[David Hart]

I did not pay a lot of attention to this renaming because I thought the trailing closure syntax would shield me from labels. But I forgot that in if/for/while statements, I’m going to be forced to use them. In those cases, I’d prefer more succinct labels.

I added some comments inline where I have issues with the new labels:

> On 23 Jun 2016, at 04:02, Erica Sadun via swift-evolution <swift-evolution at swift.org> wrote:
> 
> On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> wrote:
>> 
>> 
>> Hi All,
>> 
>> 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 <https://github.com/apple/swift/pull/2981>.  Let's please
>> carry on further discussion here rather than in the pull request, though.
> 
> -  /// - `isEquivalent(a, a)` is always `true`. (Reflexivity)
>  -  /// - `isEquivalent(a, b)` implies `isEquivalent(b, a)`. (Symmetry)
>  -  /// - If `isEquivalent(a, b)` and `isEquivalent(b, c)` are both `true`, then
>  -  ///   `isEquivalent(a, c)` is also `true`. (Transitivity)
>  +  /// - `areEquivalent(a, a)` is always `true`. (Reflexivity)
>  +  /// - `areEquivalent(a, b)` implies `areEquivalent(b, a)`. (Symmetry)
>  +  /// - If `areEquivalent(a, b)` and `areEquivalent(b, c)` are both `true`, then
>  +  ///   `areEquivalent(a, c)` is also `true`. (Transitivity)
> 
> I like this change!
> 
> -    func forEach<S: SequenceType>(_ body: (S.Iterator.Element) -> ())
> +    func forEach<S: SequenceType>(invoke body: (S.Iterator.Element) -> ())
> 
> Adding an external label makes sense here. This is a procedural call and
> using it within the parens should have a "code ripple".
> 
> That said, would prefer `do` or `perform` over `invoke` or `invoking` as in
> `runRaceTest`, `_forAllPermutationsImpl`, `expectFailure`, etc. This also applies
> where there's a `body` label instead of an empty external label.
> 
> -public func withInvalidOrderings(_ body: ((Int, Int) -> Bool) -> Void) {
> +public func withInvalidOrderings(invoke body: ((Int, Int) -> Bool) -> Void) {
> 
> For any with/external label pair, I'd prefer `with-do` or `with-perform` 
> over `with-invoke`.
> 
> -  return IteratorSequence(it).reduce(initial, combine: f)
> +  return IteratorSequence(it).reduce(initial, accumulatingBy: f)
> 
> For `reduce`, I'd prefer `applying:` or `byApplying:`
> 
> Similarly in `starts(with:comparingBy:)`, I'd prefer byComparing`,
> min/max, byOrdering
> 
> -      ).encode(encoding, output: output)
> +      ).encode(encoding, sendingOutputTo: processCodeUnit)
> 
> How about `exportingTo`?

I know this label does not need to be necessarily short, but I’d prefer Erica’s suggestion, or even `to`. encode(a, to: b) reads well to me.

> -  tempwords.sort(isOrderedBefore: <)
> +  tempwords.sort(orderingBy: <)
> 
> With `sort` and `sorted`, I'd prefer `by:`

I also agree that `by` is more than enough. `sort` already implies an `ordering`.

> -  if !expected.elementsEqual(actual, isEquivalent: sameValue) {
> +  if !expected.elementsEqual(actual, comparingBy: sameValue) {
> 
> I'm torn on this one. I don't like but I don't have a good solution.

This one has a noticeable chance of appearing in conditional clauses so I’d like to go for something short. How about `by`?

> -  /// for which `predicate(x) == true`.
> +  /// for which `isIncluded(x) == true`.
> -      _base: base.makeIterator(), whereElementsSatisfy: _include)
> +      _base: base.makeIterator(), suchThat: _include)
> 
> How about simply `include` for both? I get the `is` desire but it's being tossed away
> in a lot of other places in this diff. and `suchThat` feels out of place.
> 
> -      || u16.contains({ $0 > 127 || _isspace_clocale($0) }) {
> +    || u16.contains(elementWhere: { $0 > 127 || _isspace_clocale($0) }) {
> 
> I assume the challenge here is differentiating contains(element) from contains(closure).
> This feels predicate-y, which is why I put it near the predicates. But I think something
> like `containsElement(where:)` works better.

Agreed. Again, same argument. As this is a boolean function, this has a higher chance of appearing in conditional clauses so should be short. `where` works well for me.

>  -    let result = try base._withUnsafeMutableBufferPointerIfSupported(body)
> +    let result = try base._withUnsafeMutableBufferPointerIfSupported(invoke: body)
> 
> I hate "ifSupported" but that's another discussion (withSupportedUnsafeMutableBufferPointer,
> withAvailableUnsafeMutableBufferPointer, it's all lipstick)
> 
> This is procedural, so `do` or `perform` rather than `invoke`
> 
> -      for test in removeFirstTests.filter({ $0.numberToRemove == 1 }) {
> +      for test in removeFirstTests.filter(
> +        suchThat: { $0.numberToRemove == 1 }
> 
> The difference between `filter` and `forEach` is that `forEach` is explicitly 
> procedural while `filter` is functional.  I do not like functional chainable
> calls being modified to use explicit external labels in this way. 
> 
> I'd prefer no label here.

Quick aside. Eric, if you prefer no label here, why did you not also prefer no labels for contains and elementsEqual? They also have very functional. But if we must have a label, I’d vote for `where`.

> public func split(
>       maxSplits: Int = Int.max,
>       omittingEmptySubsequences: Bool = true,
> -    isSeparator: @noescape (Base.Iterator.Element) throws -> Bool
> +    separatedWhere isSeparator: @noescape (Base.Iterator.Element) throws -> Bool
> 
> I'm torn on this one. It's not the worst ever but something more like where/at/when
> makes more sense to me than "separatedWhere/separatedAt/separatedWhen”.

I care less about this one, but `where` also sounds better.

> +      count: __manager._headerPointer.pointee.count)
> 
> For the sake of Zippy the Pinhead, surely there has to be something better than `pointee`.
> Like...`reference`?
> 
> 
> _______________________________________________
> 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/20160623/6806c4e5/attachment.html>