Stdlib closure argument labels and parameter names


(Dave Abrahams) #1

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.

I'll be submitting a formal proposal for review this week but wanted to
give people a chance to weigh in on some of the changes first. I view
this step as a necessary one before taking up Patrick Pijnappel's
proposal for renaming functional algorithms:
http://news.gmane.org/find-root.php?message_id=CAKA%3DjdbxiC_q0Fe%3DXc%3DmSWu7OtGxMgAQyb0QMA6_OaiiLB3foA%40mail.gmail.com

Thanks,

···

--
Dave


(Brent Royal-Gordon) #2

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.

···

--
Brent Royal-Gordon
Architechies


(Erica Sadun) #3

- /// - `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`?

- tempwords.sort(isOrderedBefore: <)
+ tempwords.sort(orderingBy: <)

With `sort` and `sorted`, I'd prefer `by:`

- 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? 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.

- 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.

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".

+ count: __manager._headerPointer.pointee.count)

For the sake of Zippy the Pinhead, surely there has to be something better than `pointee`.
Like...`reference`?

···

On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution <swift-evolution@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.


(Anton Zhilin) #4

Dave Abrahams via swift-evolution <swift-evolution@...> writes:

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.

I don't like that more clutter is added to the call site. We can surely
find shorter and more descriptive names. Remember that most people
already know what filter, map and reduce means, we don't need to explain
them.

My suggestions:

filter(where:)
map(over:)
reduce(applying:)
sort(by:)
forEach(do:)

Remember that these functions came from functional languages, where
function names tend to be brief, and it is considered one of their
advantages.

Data flow is an area where code becomes the less understandable when
more visual clutter is added. In my opinion,

array.filter(isEven).map(square).reduce(sum)

reads better than

array.filter(suchThatTrue: isEven).map(applyingTransformation:
square).reduce(accumulatingResultBy: sum)

What do you think?


(David Hart) #5

Didn’t have time to give it a thorough read, but `accumulatingResultBy` did stand out negatively for me as being a bit too verbose.

···

On 21 Jun 2016, at 01:52, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

* 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.)


(Dave Abrahams) #6

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


(Xiaodi Wu) #7

Let's not go through more churn with `pointee`. It's already just been
changed from `memory`.

···

On Wed, Jun 22, 2016 at 21:02 Erica Sadun via swift-evolution < swift-evolution@swift.org> wrote:

On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution < > swift-evolution@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".

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`?

*- tempwords.sort(isOrderedBefore: <)*
*+ tempwords.sort(orderingBy: <)*

With `sort` and `sorted`, I'd prefer `by:`

*- 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? 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.

* - 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.

*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".

*+ 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@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(David Hart) #8

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:

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".

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.

···

On 23 Jun 2016, at 04:02, Erica Sadun via swift-evolution <swift-evolution@swift.org> wrote:
On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

+ 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@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Vladimir) #9

Dave Abrahams via swift-evolution <swift-evolution@...> writes:

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.

I don't like that more clutter is added to the call site. We can surely
find shorter and more descriptive names. Remember that most people
already know what filter, map and reduce means, we don't need to explain
them.

My suggestions:

filter(where:)
map(over:)
reduce(applying:)
sort(by:)

Do you mean *sorted*(by:) ? Or I'm missing something in naming rules?

forEach(do:)

Remember that these functions came from functional languages, where
function names tend to be brief, and it is considered one of their
advantages.

+1

Data flow is an area where code becomes the less understandable when
more visual clutter is added. In my opinion,

array.filter(isEven).map(square).reduce(sum)

reads better than

array.filter(suchThatTrue: isEven).map(applyingTransformation:
square).reduce(accumulatingResultBy: sum)

What do you think?

+100. I even want to brought the term-of-art argument here. IMO These functions are expected to be called without any parameter names.

From other point, often these functions will be called with closures(i.e. without parameter names at all):

array.filter {$0 % 2 == 0} .map {$0*$0} .reduce(0) {$0+$1}

···

On 24.06.2016 0:55, Anton Zhilin via swift-evolution wrote:

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Patrick Smith) #10

Yes, I think the long names somewhat reduces the argument for keeping ‘terms of art’, as in this proposal the method names are describing themselves twice. Is the extra clarity for those who are not familiar with `filter`, `map`, `reduce` etc from other languages? Is this extra clarity actually helping — is someone going to say ‘ah it’s applying the transformation, now it makes sense’. From experience, these methods make sense after using them for the first time, not reading them in the documentation where even the longer description can feel quite abstract.

e.g. ‘accumulatingResultBy’ does not tell me much more than ‘reduce’, it’s probably not until they see an example or play around with it that someone new sees the utility of what it does and how it works.

Patrick

···

On 24 Jun 2016, at 7:55 AM, Anton Zhilin via swift-evolution <swift-evolution@swift.org> wrote:

array.filter(isEven).map(square).reduce(sum)

reads better than

array.filter(suchThatTrue: isEven).map(applyingTransformation:
square).reduce(accumulatingResultBy: sum)

What do you think?


(Dave Abrahams) #11

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`?

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.

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.

- ).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})

- 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.

- 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?

···

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@swift.org> wrote:

--
-Dave


(Dave Abrahams) #12

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,

···

on Tue Jun 21 2016, Dave Abrahams <swift-evolution@swift.org> wrote:

on Mon Jun 20 2016, Brent Royal-Gordon <swift-evolution@swift.org> wrote:

--
Dave


(Erica Sadun) #13

contains and elementsEqual both return Booleans which are almost never chained.

-- E

···

On Jun 23, 2016, at 12:39 AM, David Hart <david@hartbit.com> wrote:

- 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`.


(Anton Zhilin) #14

Vladimir.S via swift-evolution <swift-evolution@...> writes:

Do you mean *sorted*(by:) ? Or I'm missing something in naming rules?

IIRC, sort is mutating and sorted is nonmutating (copying) version.
Also, I forgot `initial` parameter in `reduce`.

> Data flow is an area where code becomes the less understandable when
> more visual clutter is added. In my opinion,
>
> array.filter(isEven).map(square).reduce(sum)
>
> reads better than
>
> array.filter(suchThatTrue: isEven).map(applyingTransformation:
> square).reduce(accumulatingResultBy: sum)
>
> What do you think?

+100. I even want to brought the term-of-art argument here. IMO These
functions are expected to be called without any parameter names.

That would probably be a good scenario, but core team needs to release
their grip on strictly following new naming conventions.


(Charlie Monroe) #15

Vladimir.S via swift-evolution <swift-evolution@...> writes:

Do you mean *sorted*(by:) ? Or I'm missing something in naming rules?

IIRC, sort is mutating and sorted is nonmutating (copying) version.
Also, I forgot `initial` parameter in `reduce`.

Data flow is an area where code becomes the less understandable when
more visual clutter is added. In my opinion,

array.filter(isEven).map(square).reduce(sum)

reads better than

array.filter(suchThatTrue: isEven).map(applyingTransformation:
square).reduce(accumulatingResultBy: sum)

What do you think?

+100. I even want to brought the term-of-art argument here. IMO These
functions are expected to be called without any parameter names.

That would probably be a good scenario, but core team needs to release
their grip on strictly following new naming conventions.

I know it might not exactly by the naming conventions, but have you considered

filter(predicate:) // Though it might get confused with (NS)Predicate?
map(transform:)
reduce(accumulator:)

All of this may be extended to follow the convetions:

filter(usingPredicate:)
map(usingTransform:)
reduce(usingAccumulator:) // or applyingAccumulator?

But I agree the label-less argument here reads best.

···

On Jun 24, 2016, at 4:11 PM, Anton Zhilin via swift-evolution <swift-evolution@swift.org> wrote:

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Dave Abrahams) #16

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:

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".

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.

Makes sense.

- 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`.

See my reply to Erica.

- 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`?

You know, it's not bad. Maybe it's just the fever talking but I am
starting to like these short labels.

- /// 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.

  if friends.contains(where: {$0.firstName == "ted"}) { ... }

? `contains(where:` doesn't seem very fluent to 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`.

IMO `where` fails to correct one of the great weaknesses of the basename
`filter`, which is the strong implication that the predicate's meaning
is the inverse of what it actually is. That's why I favor `soEach`.

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.

As I noted in my reply to Erica, I think `where` without some reference
to the separator is potentially misleading.

Perhaps

    contiguousNonPrimes = (0..<100).split(whereSeparator: isPrime)

would be better for use sites than what's in the patch, though.

···

on Wed Jun 22 2016, David Hart <david-AT-hartbit.com> wrote:

On 23 Jun 2016, at 04:02, Erica Sadun via swift-evolution <swift-evolution@swift.org> wrote:
On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution >> <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> >> wrote:

--
-Dave


(Erica Sadun) #17

- 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.

When using a procedural "trailable" closure inside parentheses, the intention
to do so should be clear:

p(x, perform: {...})
p(x, do: {...})

vs

p(x) {
   ...
}

Anyone reading this code can immediately identify that an otherwise trailing
closure has been pulled inside the signature because the call has become
significantly more complex. The point-of-coding decision ripples through
to point-of-reading/point-of-maintenance.

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 read through the pull request. I grouped related modifications
together, although not exhaustively. They fall under the same umbrella,
choosing `do` or `perform` compared to `invoke` or `invoking`.

- _forAllPermutationsImpl(index + 1, size, &perm, &visited, body)
+ _forAllPermutationsImpl(index + 1, size, &perm, &visited, invoke: body)

-public func runRaceTest(trials: Int, threads: Int? = nil, body: () -> ()) {
+public func runRaceTest(
+ trials: Int, threads: Int? = nil, invoking body: () -> ()
+) {

-public func expectFailure(${TRACE}, body: () -> Void) {
+public func expectFailure(${TRACE}, invoking body: () -> Void) {

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.

You did, as in the examples just above, which you recommend a rename to `invoke` or
`invoking`.

-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?

Strong personal taste, backed by tech writing. Simpler words.
From the Corpus of Contemporary American English:
do: 18
perform: 955
invoke: does not appear on list.

- return IteratorSequence(it).reduce(initial, combine: f)
+ return IteratorSequence(it).reduce(initial, accumulatingBy: f)

For `reduce`, I'd prefer `applying:` or `byApplying:`

Makes sense.

Similarly in `starts(with:comparingBy:)`, I'd prefer byComparing`,

I don't see how that works. You're not comparing the closure with
anything.

I get your point but I think it's unnecessarily fussy.

Alternatives are slim on the ground. `usingComparison:` is too long,
as is `byComparingWith:` (which still reads better but you will point
out can be mistaken by some pedant to mean that the sequence is being
compared to the closure), and you won't allow for `comparing:`.
I'm not in love with `comparingWith:` but it reads slightly better to me
than `comparingBy:`.

min/max, byOrdering

Likewise, you're not ordering the closure.

Same reasoning.

- ).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.”

For a language that treasures concision and clarity, this may be clear
but it's notably inconcise. (Yes, that is a new word.)

- 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})

But they're sorting now, and that feature isn't in Swift.

However, `sorted` falls under another umbrella, which is potential
chaining. In such case, I would prefer there be no internal label at all
to allow cleaner functional flow.

- 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.

I actually meant to move this up to the other discussion of `byComparing` and
forgot to. So please assume you disagree with me on this one too.

- /// 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 like `predicate`. I endorse `predicate`. Does this mean the rule of "must
read like a sentence" can be overturned for things like "comparison" and
"order"? If so, woo!

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.

To quote greatness:, "I personally find `soEach` repulsive".
(cite: http://article.gmane.org/gmane.comp.lang.swift.evolution/16998/match=repulsive)

- || 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.

May I recommend `predicate:` then since it looks like that's actually a possibility?

- 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.

Woody Allen: "The heart wants what it wants"
Me: "The spleen vents what it vents"

(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?

Functional programming flow. I follow Kevin Ballard's rule of parens around
functional elements and naked braces for trailing closures that do not return
values. This ensures the compiler is never confused at places like:

for x in foo when y.f{...} {
    ...
}

and instantly identifies to the reader when there's a non-returning scope, as
in forEach or GCD dispatch.

However, if you use chaining and if the language insists on external label
preambles, which would not be seen when using the call with a trailing closure,
it becomes far less readable and encourages people to use trailing closures to
avoid the label, i.e. an attractive nuisance. Simple selectors encourage better fp:

let x = myStream.f1({...}).f2({...})

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.

`predicate` ftw.

+ 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?

It was humor. It was at the end. I assumed the joke would lighten the
previous complaints and bookend the positive support at the start of
my message.

--
-Dave

-- E

···

On Jun 25, 2016, at 4:25 PM, Dave Abrahams <dabrahams@apple.com> 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@swift.org> wrote:


(David Hart) #18

Comments inline:

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.

···

Sent from my iPhone

On 26 Jun 2016, at 00:25, Dave Abrahams via swift-evolution <swift-evolution@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@swift.org> wrote:

- 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@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(David Rönnqvist) #19

Vladimir.S via swift-evolution <swift-evolution@...> writes:

Do you mean *sorted*(by:) ? Or I'm missing something in naming rules?

IIRC, sort is mutating and sorted is nonmutating (copying) version.
Also, I forgot `initial` parameter in `reduce`.

Data flow is an area where code becomes the less understandable when
more visual clutter is added. In my opinion,

array.filter(isEven).map(square).reduce(sum)

reads better than

array.filter(suchThatTrue: isEven).map(applyingTransformation:
square).reduce(accumulatingResultBy: sum)

What do you think?

+100. I even want to brought the term-of-art argument here. IMO These
functions are expected to be called without any parameter names.

That would probably be a good scenario, but core team needs to release
their grip on strictly following new naming conventions.

I know it might not exactly by the naming conventions, but have you considered

filter(predicate:) // Though it might get confused with (NS)Predicate?
map(transform:)
reduce(accumulator:)

All of this may be extended to follow the convetions:

filter(usingPredicate:)
map(usingTransform:)
reduce(usingAccumulator:) // or applyingAccumulator?

But I agree the label-less argument here reads best.

Depending on how the API Guidelines (specifically about “omit needless words”) is read and if one considers a function (A) -> Bool to be a predicate and a function (A) -> B to be a transform, then the argument could be made that the words “predicate” and “transform” should be omitted. This only leaves the label “using” in both these cases (which I find to be too general and not very descriptive).

- David

···

On 24 Jun 2016, at 16:26, Charlie Monroe via swift-evolution <swift-evolution@swift.org> wrote:

On Jun 24, 2016, at 4:11 PM, Anton Zhilin via swift-evolution <swift-evolution@swift.org> wrote:

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Dave Abrahams) #20

No such grip exists. We prefer to follow the API guidelines where
possible, but this discussion is about figuring out what's right to do
for *these APIs*.

···

on Fri Jun 24 2016, Anton Zhilin <swift-evolution@swift.org> wrote:

+100. I even want to brought the term-of-art argument here. IMO These
functions are expected to be called without any parameter names.

That would probably be a good scenario, but core team needs to release
their grip on strictly following new naming conventions.

--
Dave