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 avoid this forum or just keep your feedback private, directly to me via email, twitter, or direct message. If emailing me directly, please put "SE-0204: Add last(where:) and lastIndex(where:) methods" in the subject line. Your feedback is equally valuable to us no matter how we receive it.
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?
Get's a +1 from me. Seem like logical companion methods to .first and .index.
However, I kind of wish this was put back in the proposal after all:
Another previous proposal included renaming index(of:) and index(where:) to firstIndex(of:) and firstIndex(where:), respectively. This version of the proposal removes that source-breaking change.
Makes sense to add these. Sharing the same complaint as others in the thread, I too would like to see at least a consideration on modernising the naming of the existing index(where:) to match this proposal. Is adding firstIndex(where:) in this release, and eventually deprecating index(where:), really untenable?
There's the source stability answer to this, and then there's to-heck–with-source-stability answer. Both point towards leaving index(where:) untouched IMO.
index(where:) is a long-standing and very heavily-used method, and renaming it purely for consistency with lastIndex(where:) would not be in keeping with our high bar for source stability at this stage of Swift. To justify the rename, it would need some other additional motivation than just consistency (for example, the active confusion and misuse that led to the compactMap rename).
But even if we didn't have a source stability requirement, I would still argue that index(where:) is the most appropriate name. Searching forward for an index is extremely common, whereas needing to search backwards is useful but relatively uncommon (especially given a lazily reversed() collection is often a better way of doing what you want). Given this, the added verbosity of making it firstIndex(where:) seems gratuitous to me. Symmetry is pleasing, which is sometimes an OK naming justification, but in this case it harms more than it helps.
If users could mistakenly think it would find all the matching indices, there might be an argument of avoiding confusion. But the return type of Index? makes it very clear it's only returning one value. And I don't think there's much ground for not thinking it would do anything other than find the first matching index (rather than an arbitrary one, or the last one).
Devil's advocate: a user might misinterpret index(where:) as behaving like what one might otherwise name onlyIndex(where:)--i.e., a method that returns nil if there's either zero or more than one element matching the argument.
Reasonable arguments here. On the scale of harm, it isn't huge (unlike rationalising the sequence operations, which I really hope will be revived and addressed). I still find the asymmetry irksome:
| | |
|---|---|---|
| first | last |
| first(where:) | last(where:) |
| index(where:) | lastIndex(where:) |
I think if you consider that table of methods, newcomers may be led astray believing index(where:) is not guaranteed to return from the front. Regardless, I'm not going to press hard on this one.
I think I agree with this — index(of:) and index(where:) are probably fine as they are now, but with the addition of lastIndex(of:) and lastIndex(where:), there's potential for some confusion that changing the names would help clarify. In any case, I think that question is (or at least could be) taken up by the proposal you linked.
Don't get me wrong: I entirely agree. That said, I've also been burned by making that assumption: users' minds often turn out to be more imaginative than one anticipates.
Okay, so far the consensus seems to be strongly in favor of what's in the actual proposal. People are split about whether we should also take the opportunity to rename index(where:) to firstIndex(where:). That's not in the proposal, but it seems so closely related that I don't mind people talking about it. Just so I know, though, is there anybody who feels like they would oppose this proposal if index(where:) was not renamed? Is there anybody who doesn't like this proposal for other reasons?
Although I prefer index over firstIndex because of source compatibility reasons, I want to share a potentially confusing case for beginners.
One may assume the index function will magically return the correct index in a for in loop.
for element in array {
let index = array.index(of: element)
// do something
}
This above implementation is wrong if the array contains duplicated elements. In this case, firstIndex is a much better name.
If both element and index are needed, the code can be implemented as:
for index in 0..<array.count {
let element = array[index]
// do something
}
However, one cannot use the subscript syntax to fetch a character from a string. Beginners dealing with string may tend to use the first (wrong and inefficient) implementation.
It’s harder to come up with the enumerated() or even zip() solution.
for (counter, element) in array.enumerated() {
// do something
}
for (index, element) in zip(array.indices, array) {
// do something
}
let crazyString = "ZA̡͊͠͝LGΌ ISͮ̂҉̯͈͕̹̘̱ TO͇̹̺ͅƝ̴ȳ̳ TH̘Ë͖́̉ ͠P̯͍̭O̚N̐Y̡ H̸̡̪̯ͨ͊̽̅̾̎Ȩ̬̩̾͛ͪ̈́̀́͘ ̶̧̨̱̹̭̯ͧ̾ͬC̷̙̲̝͖ͭ̏ͥͮ͟Oͮ͏̮̪̝͍M̲̖͊̒ͪͩͬ̚̚͜Ȇ̴̟̟͙̞ͩ͌͝S̨̥̫͎̭ͯ̿̔̀ͅ"
for i in crazyString.indices {
let character = crazyString[i]
print(i, character)
}