Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager via email or direct message. When emailing the review manager directly, please keep the proposal link at the top of the message.
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
The motivation section does not motivate me much. Yes elementsEqual is not a fantastic name. But I find the confusion argument with == a little made-up, as if someone was looking for something to chew on. What should we say about ===, which is a single keystroke from ==?
Is the problem being addressed significant enough to warrant a change to Swift?
All right, if we change for something better.
Does this proposal fit well with the feel and direction of Swift?
I know that API guidelines favor clarity. No one could tell that elementsEqualInIterationOrder is not clear. OK for the "direction".
But not for the "feel". I'd suggest instead a name that clearly names the kind of equality we talk about: a.sequenceEqual(b).
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
To anyone who tells that sequenceEqual does not fit the API design guidelines, I'll answer that elementsEqualInIterationOrder is worse. Most other languages don't have a closed API design guideline that sometimes hinders API legibility and... swiftness :-)
But I could live with elementsEqualInIterationOrder, of course.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Since the proposal is a name change, are we allowed to bikeshed the name???
One minor problem with "in iteration order" is that it's not entirely obvious what the iteration order is. A user might think this is something that somehow needs to be specified, something like a sort order. In that regard, a clearer name might be elementsEqualInSequenceOrder, but I agree with others that this kind of phase is a bit of a mouthful.
A mathematician might describe this kind of equality as sequences being "elementwise equal". In that spirit, I'd suggest calling the method elementwiseEquals.
I don't want to pretend I know what's in the proposal authors' mind, but I think that they find a problem in that the word "element" is context-sensitive. setA.elementsEqual(setB) reads as if we were comparing set elements (which we don't).
If my interpretation above is correct, elementwiseEquals does not bring much disambiguation. In setA.elementwiseEquals(setB), we still look as if we are talking about set elements (when we don't).
That's why I suggest sequenceEquals: so that there is not any ambiguity whatsoever on the kind of equality we talk about. In setA.sequenceEquals(setB), we compare sequences (obviously).
A criticism of other names originally pitched was that users would think the method first sorted the elements, then compared the elements. The same deficiency could apply to “elements equal in order”—hence, the clarification. I did not find this critique to be overly likely, but feedback in this forum was that it was very likely and must be avoided.
It is not made up: the proposal stems from actual real-world confusion reported by others.
If we could find a nice short name that describes the semantics better, then it might be desirable. However, the proposed spelling sacrifices far too much conciseness to be worth the extra specificity.
For the vast majority of sequences the order of the elements is intrinsically important, so the name change would not bring any benefit to them. Yet they would still bear the cost of its added verbosity.
It is only for “unordered” sequences (eg. Set, Dictionary, etc.) that confusion could arise. I think the hammer being brought to bear is too heavy for the problem under consideration.
Not quite. The motivation section contains a report by Ole Begemann, who is not somebody who is easily confused, and an extensive quote form Michael Ilseman, who writes a very detailed analysis of sequence equality. These are not "real-world confusion". These are analysis of a problem whose existence is not quite established.
Very good. SR-5521 should be linked from the motivation section of the proposal, so that readers can see it (and the replies).
Now it does still not mean that the problem needs addressing. We all face questions from users who don't read the documentation when they meet a behavior that they do not expect. This is not, per se, a sign of a faulty API.
[can't fight the feeling of deja vu...]
Imho a method whose name consists of five words doesn't look like a good fit for the stdlib, and it would be better to let people who need the functionality write it themselves.
But I think the problem is bigger than a single name:
The operation in question relies on the order of elements, which is a concept that's alien to collections like sets.
Therefore, I don't think there is any legitimate use case for elementsEqualInIterationOrder for such collections.
So for me, the only clean solution is not a renaming in the method space, but rather fixing the protocol hierarchy.
The quick and dirty way would be something like DeterministicSequence, but that makes me shiver as well...
The clean solution would that Set itself doesn't conform to Sequence, but has a property that does (because a set isn't a sequence, but you can construct a sequence from a set).
I haven't thought much about problems a "correct" system of collection protocols would have, because I don't think such a radical move will ever happen - but I guess @xwu has a link to the matching discussion ;-)
Looks like I'm in the same boat as most people here: function definitely needs renaming, but proposed name is kinda gross.
I'll throw out my suggestion: elementsSequentiallyEqual(). Sure, this is defined on Sequence so it initially may seem redundant, but when used on actual types that simply conform to Sequence, I think it conveys meaning well:
set1.elementsSequentiallyEqual(set2)
Alternatively, I also suggest elementsEqualSequentially, but I have a slight less preference for it.
I also like sequenceEquals. It strongly implies equality of elements in the order the sequence presents them. This captures the important distinction from == without being unduly verbose. It is the first solution to this rather thorny problem that I don't have any qualms about endorsing.
If sequenceEquals has support, I'd suggest the slight variation sequentiallyEquals as a bit more grammatically harmonious when the expression is read in full ("x sequentially equals y", rather than "x sequence-equals y").
@gwendal.roue The mistake you quoted in your subsequent post was exactly the one that I had made when I wrote the tweet@xwu linked to in the proposal:
I needed to compare two Sets for equality in a unit test, and since Set wasn’t Equatable pre-Swift-4.1 and XCTest didn't have an overload of XCTAssertEqual for sets, elementsEqual seemed like the straightforward solution.
Now that we have conditional conformance, this particular mistake won’t come up as much going forward, but the potential for misuse remains in other situations.
+1 on the proposal. I don't have a strong opinion on the name.
I needed to compare two Sets for equality in a unit test, and since Set wasn’t Equatable pre-Swift-4.1 and XCTest didn’t have an overload of XCTAssertEqual for sets, elementsEqual seemed like the straightforward solution.
Thanks Ole! The fact that Set wasn't equatable was indeed, now I see it, a huge push for elementsEqual and its trap. It's crystal clear now.
Now that we have conditional conformance, this particular mistake won’t come up as much going forward, but the potential for misuse remains in other situations.
Thanks again, for your honesty. We're in a post-4.1 world now, and setA == setB has become forever available. elementsEqual has lost this opportunity of being confusing.
Sorry if I push for more motivation - this does not mean I don't want this proposal to be implemented. I just want the proposal to better explain its motivation. I'll be happy if sequenceEquals or a variant replaces elementsEqual.