Make Iterator naming more consistent #11001
Conversation
|
I think this should be discussed as to whether this is the right direction to go in. |
|
I don't have an opinion as to what naming convention is better. Is this the appropriate place for that discussion to occur or is ML better? |
|
I rebased to origin/master and also renamed all Iterators without a suffix (that I could find) to have a suffix of Iterator. I didn't do extensive testing since I mostly mean for this to be an RFC at this point. |
|
So, I said I was giving up on this on the mailing list, but I decided to give it one more go. In my attempt to get rid of an eye-sore that I feel responsible for adding, I've ended up with this PR that renames almost every existing iterator. Anyway, I removed the "Iterator" suffix from every iterator, but left the traits in iter.rs untouched. I tried to rename all of the iterators to be more descriptive over what they are iterating. So, I renamed Anyway, here is a chart with all the renaming that I did. I think the chart is almost in sync with the pull request, but I don't guarantee they are 100% the same. The chart is better for critiquing (or even better - suggesting improved names) than for reviewing the pull request:
In closing, I hope I never see an iterator again. |
|
Ohh I like this quite a bit. Actually puts the module system to use! I was indifferent to the earlier renamings, thinking all they did was shuffle things about needlessly, but these names feel right to me. |
|
Items is shorter than Elements |
|
If the consensus is that 'Items' is better than 'Elements', I'm happy to change it up. Personally, I prefer 'Elements' a bit more than 'Items', but I don't have a strong opinion either way. Due to type inference, the vast majority of these types are never actually referenced outside of the module that they are declared in so, having a slightly longer name isn't much of a burden for the average user. |
|
/me joins the bikeshed I'm very happy with Elements: one normally says "the elements of a vector" not "the items of a vector" (at least, that's what I say) and one doesn't write the iterator types out much anyway. |
|
Abbreviating |
|
No, it's not "the Elements of a vector", it is "the Items from a iterater".
|
|
These are iterators over the elements of various data-structures. (If we called it "items" as in "items of an iterator" then we could call every iterator "items", because all iterators have items.) |
|
Rebased after #11321 landed to fix merge conflicts and rename the new iterator. I also updated my comment above with all of the details on what iterators were renamed to. |
|
Ok, cool, np. I'll rebase again after that lands. |
|
I am adding this to the next meeting agenda |
|
(thanks for the renaming overview, it's very convenient to have!) |
|
Rebased and updated my comment above for the new iterator, TrieMapMutIterator -> MutPairs. Thanks for adding it to the agenda! |
|
@DaGenix would you be willing to do the same treatment to Servo as well? :) |
|
We discussed this at today's meeting, and we're in favor of the general removal of the name iterator. We felt that Items was slightly better than Elements because it is indeed shorter and elements/items are essentially the same thing. One thing I'm also interested in establishing a set of naming conventions for iterators. From your renamings, I get this impression:
Do those rules sound reasonable to you? I'll add them to the style guide once this lands. |
|
@alexcrichton Sounds good to me! I'll work on rebasing this to change Elements to Items. That is what I was thinking regarding rules when I did the renaming. My thinking with "SetElements" (from treemap / hashmap) is that just "Elements" would be confusing because those modules define two separate types - a Map type and a Set type - and it would not be all that clear which of those an iterator "Elements" belonged to. So, I broke the general rules you outlined above because I think "SetElements" is easier to read for that reason. So, if that makes sense, maybe their is a final rule:
@metajack I'll will take a look a Servo. I've never built it before, so, we'll see how that goes. |
|
Sounds good to me, and thanks so much for doing this work! I really like this PR :) |
|
Thanks, np. This ended up being quite a bit bigger than I originally intended, but I'm quite happy with the result. |
|
Rebased to change Elements -> Items. |
|
I feel fairly strongly that |
|
I agree with @tikue here. Java uses |
|
Sounds reasonable to me, @DaGenix, would you mind renaming pairs to entries? |
|
If the iterator is named after the iterated elements, I'd expect |
|
Sounds good, I will rebase with changes. It makes sense to me that BitvIterator becomes Bits. But what does BitvSetIterator become? |
|
Given that |
|
According to our rules above, the |
|
That would break the first rule. BitvSet yields bit indices (as uints |
|
Rebased again to rename Pairs -> Entries, bitv::Items -> bitv::Bits, and bitv::SetItems -> bitv::BitPositions. Thanks for everyone's input! |
|
This needs another rebase, unfortunately. (Probably my most recent batch of trie iterator changes... sorry :( ) |
Rename existing iterators to get rid of the Iterator suffix and to give them names that better describe the things being iterated over.
|
Ok, cool, np. Rebased. |
Most Iterators renamed to make their naming more consistent. Most significantly, the Iterator and Iter suffixes have been completely removed.
|
I have added a note to our style guide about this https://github.com/mozilla/rust/wiki/Note-style-guide#iterators |
[`question_mark`]: don't lint inside of `try` block Fixes rust-lang#8628. Diff looks a bit noisy because I had to move the two functions into an impl, because they now need to access the structs `try_block_depth` field to see if they're inside a try block. changelog: [`question_mark`]: don't lint inside of `try` block
Most Iterators renamed to make their naming more consistent. Most significantly, the Iterator and Iter suffixes have been completely removed.