-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adds entry_index to map.rs #115
Conversation
cd4ae70
to
008828f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me. Can you also add an equivalent method on IndexSet
?
@cuviper, I'm sorry I haven't gotten back to you on this, I was sick over the last week and just genuinely forgot, I'll get on this as soon as I'm able! |
Ok, Made the requested changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Wait - this method seems underspecified and surprising? Both the PR and the doc comment needs to explain in detail what it does. The method should not be called |
Well, at least it's not published yet if you want to change/revert it. Yes, the main advantage I see is to avoid the indexing operation in |
I am sorry for my tone there, that's certainly not pitch perfect, and extremely inappropriate for a "maintainer" that is as AWOL as I am. Name doesn't need to be perfect, but it's clear that the current name is confusing, and it would be best to find a better doc comment or name before release - but it's not a requirement either. |
I opened issue #119 for this. |
I wanted a way to access the index but the only way to do that is by getting the entry and then the index from that which is mutable (and causes issues) or from
get_full
which returns more than what I needed so I addedentry_index