Comments inline:
Sent from my iPhone > On 26 Jun 2016, at 00:25, Dave Abrahams via swift-evolution > <[email protected]> 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 >>> <[email protected]> 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 > [email protected] > https://lists.swift.org/mailman/listinfo/swift-evolution _______________________________________________ swift-evolution mailing list [email protected] https://lists.swift.org/mailman/listinfo/swift-evolution
