Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds entry_index to map.rs #115

Merged
merged 4 commits into from
Feb 12, 2020

Conversation

Thermatix
Copy link
Contributor

@Thermatix Thermatix commented Jan 31, 2020

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 added entry_index

@Thermatix Thermatix changed the title Adds get_index to map.rs Adds entry_index to map.rs Jan 31, 2020
Copy link
Member

@cuviper cuviper left a 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?

src/map.rs Outdated Show resolved Hide resolved
@Thermatix
Copy link
Contributor Author

@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!

@Thermatix
Copy link
Contributor Author

Ok, Made the requested changes!

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@cuviper cuviper merged commit 264f811 into indexmap-rs:master Feb 12, 2020
@bluss
Copy link
Member

bluss commented Mar 15, 2020

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 entry_* when it is more similar to get_full than to entry. I can guess that the motivation for adding this is performance, just having a simpler return type than get_full, is not enough to warrant inclusion.

@cuviper
Copy link
Member

cuviper commented Mar 16, 2020

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 get_full, which seems reasonable to me. The author originally called it get_index, which might be a better name, but that method already exists for using an index to get the key-value.

@bluss
Copy link
Member

bluss commented Apr 9, 2020

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.

@cuviper
Copy link
Member

cuviper commented Apr 9, 2020

I opened issue #119 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants