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

[David Hart]

Comments inline:



Sent from my iPhone
> On 26 Jun 2016, at 00:25, Dave Abrahams via swift-evolution <swift-evolution at swift.org> wrote:
> 
> 
>> on Wed Jun 22 2016, Erica Sadun <erica-AT-ericasadun.com> wrote:
>> 
>>> On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution <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.  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".
> 
> I don't think I understand what you mean here.
> 
>> That said, would prefer `do` or `perform` over `invoke` or `invoking` as in
>> `runRaceTest`, `_forAllPermutationsImpl`, `expectFailure`, etc. 
> 
> Sorry, again, I'm a little lost.  Forgive my overly-literal brain but
> could you please spell out how those latter 3 names relate to the choice
> of `do` or `perform` over `invoke`?

I'd vote for `do`, just to keep those functional method labels short.

>> This also applies where there's a `body` label instead of an empty
>> external label.
> 
> We don't have any methods with a `body` label IIUC.
> 
>> -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`.
> 
> OK.  Is there a rationale, or is it just personal taste?
> 
>> -  return IteratorSequence(it).reduce(initial, combine: f)
>> +  return IteratorSequence(it).reduce(initial, accumulatingBy: f)
>> 
>> For `reduce`, I'd prefer `applying:` or `byApplying:`
> 
> Makes sense.

`applying` looks great. The `by` looks redundant in this case.

>> Similarly in `starts(with:comparingBy:)`, I'd prefer byComparing`,
> 
> I don't see how that works.  You're not comparing the closure with
> anything.
>> min/max, byOrdering
> 
> Likewise, you're not ordering the closure.

I agree with you Dave on both of those.

> 
>> -      ).encode(encoding, output: output)
>> +      ).encode(encoding, sendingOutputTo: processCodeUnit)
>> 
>> How about `exportingTo`?
> 
> “export” is freighted with various connotations that I don't think we
> want to make here.  In fact it doesn't mean anything more exotic than
> “sending output to.”
> 
>> -  tempwords.sort(isOrderedBefore: <)
>> +  tempwords.sort(orderingBy: <)
>> 
>> With `sort` and `sorted`, I'd prefer `by:`
> 
> When people talk about “sorting by XXX”, XXX is a property of the
> elements being sorted.  Therefore IMIO that label should probably be
> reserved for a version that takes a unary projection function:
> 
>     friends.sort(by: {$0.lastName})

How about `sort(with:)` then?

> 
>> -  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.
>> 
>> -  /// 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? 
> 
> Looking at the effect on the doc comments—which is how we should judge
> parameter names—I think `predicate` might read better.
> 
>> 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.
> 
> I think I'm pretty strongly sold on “soEach” as a label for closures in
> all the filtering components.
> 
>> -      || 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.
> 
> I understand your motivation for suggesting it.  The downside is that it
> severs the strong basename relationship between algorithms that do
> effectively the same things, one using a default comparison and the
> other using an explicitly-specified one.  I'm truly not sure where we
> ought to land on this one.

I agree with Dave, the algorithms do the same thing, but with different arguments. I think we'd loose discoverability if we applied Erica's proposition.

>> -    let result = try base._withUnsafeMutableBufferPointerIfSupported(body)
>> +    let result = try base._withUnsafeMutableBufferPointerIfSupported(invoke: body)
>> 
>> I hate "ifSupported" 
> 
> Me too.
> 
>> but that's another discussion
> 
> Quite.  It's also an internal API so it's not an evolution issue.  The
> point of having that change as part of the diff (as with all the other
> use sites), is to observe how the changes affect real usage.
> 
>> (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.
> 
> Can you provide rationale for treating functional methods differently,
> or is it just personal taste?
> 
>> 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".
> 
> The biggest reason to keep “separate” in the name is the connection with
> the semantics of the other `split` method, that takes a single element
> that is tested for equality.  I think this is very similar to the
> `contains(elementWhere` vs `containsElement(where` discussion.  If you
> leave “separate” out of the name it fails to imply that those elements
> for which the predicate returns true are not present in the result.
> 
>> 
>> +      count: __manager._headerPointer.pointee.count)
>> 
>> For the sake of Zippy the Pinhead, surely there has to be something better than `pointee`.
>> Like...`reference`?
> 
> It's not a reference; it's a value.  But that, my friend, is an
> *entirely* different discussion. Let's try to stick to the scope of the
> proposal: names and labels for parameters of function type, OK?
> 
> -- 
> -Dave
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution