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

Implement Into<usize> for NodeId #58

Merged
merged 2 commits into from
May 24, 2020

Conversation

wlinna
Copy link
Contributor

@wlinna wlinna commented May 24, 2020

Implements Into<usize> for NodeId. This implementation returns index0() since 0-based indexing is more conventional.

A more explicit alternative would be making index0 public. This would have the benefit of not being "less astonishing", since one might not expect Into returning a different value.

For background info, see the related issue

@wlinna
Copy link
Contributor Author

wlinna commented May 24, 2020

I just realized that impl fmt::Display for NodeId gives index1-based representation, so these two indeed give confusing results now. I'm not sure if I should change this though. Opinions?

@saschagrunert
Copy link
Owner

Yes I think we should return the same, so the index1 based result. :)

@wlinna
Copy link
Contributor Author

wlinna commented May 24, 2020

Should I then implement this for both NonZeroUsize and usize?

@saschagrunert
Copy link
Owner

I think it does not hurt, yeah.

Also implement Into<NonZeroUsize> for NodeId
Copy link
Owner

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@saschagrunert saschagrunert merged commit e1e5e01 into saschagrunert:master May 24, 2020
@wlinna
Copy link
Contributor Author

wlinna commented May 24, 2020

Sorry, I was too hasty, wasn't focusing well enough apparently. I just noticed on my own project, that with these two impls, I have a hard time using into.

Now I get this error

error[E0283]: type annotations needed for `indextree::id::NodeId`
   --> src/octree.rs:144:86
144 |             let children: Vec<usize> = node_i.children(&self.arena).into_iter().map(|c: NodeId| -> usize { c.into()}).collect();
    |                                                                                      ^ consider giving this closure parameter a type
    = note: cannot resolve `indextree::id::NodeId: std::convert::Into<_>`

I tried specifying types in different places, or explicitly calling Into::<usize>::into(c), and things like that, but I still get the same error.

@wlinna
Copy link
Contributor Author

wlinna commented May 24, 2020

Nevermind. On further inspection, I noticed that the problem is unrelated.

Glad to contribute. Thanks for the rapid communication!

@saschagrunert
Copy link
Owner

Sounds good, I’m glad that it helps on your side. 👍

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.

2 participants