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

Use FromIterator trait rather than custom method #648

Merged
merged 13 commits into from
Jul 20, 2019

Conversation

max-sixty
Copy link
Contributor

ref #642 (comment)

Is this the right direction?

I'm only now understanding how the rust import system works; I initially didn't realize you'd need an additional use statement each time to use from_iter. Should we be using .collect() in the examples? Or adding the std::iter::FromIterator; in the ndarray standard prelude?

@LukeMathWalker
Copy link
Member

It definitely looks like the right direction @max-sixty!

Adding std::iter::FromIterator to the prelude would indeed reduce the chance of seeing existing code break with the new version, but it would still be a breaking change.
Not everyone uses the prelude and we are still removing the old from_iter function, which I think is the right decision: keeping both would lead to confusing compiler errors, asking for disambiguation when use std::iter::FromIterator is in scope.

An important point to keep in mind when working on this PR: make sure to update ndarray for NumPy users to reflect the changes you are introducing!

@max-sixty
Copy link
Contributor Author

OK great; I updated the code throughout.

Apart from the docs, what else?

Do we need to wait for a minor version to merge the from_iter removal?

Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

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

I've added some minor comments; everything else looks good to me.

I'm only now understanding how the rust import system works; I initially didn't realize you'd need an additional use statement each time to use from_iter.

This is because from_iter is provided by the FromIterator trait, and FromIterator is not included in the standard Rust prelude (so you have to explicitly use it). In contrast, collect is provided by the Iterator trait, which is included in the standard Rust prelude.

Should we be using .collect() in the examples?

I think that's a bit nicer than adding use std::iter::FromIterator, but either way is fine.

Or adding the std::iter::FromIterator; in the ndarray standard prelude?

I don't think we should do that because IMO that would be somewhat surprising.

Do we need to wait for a minor version to merge the from_iter removal?

We should include this as part of 0.13.0, which is the next release. (We've already merged some breaking changes into master.)

src/impl_constructors.rs Outdated Show resolved Hide resolved
src/arraytraits.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
examples/life.rs Outdated Show resolved Hide resolved
src/arraytraits.rs Show resolved Hide resolved
@max-sixty
Copy link
Contributor Author

Great, thanks for the thoughts.

Updated on comments, lmk any final changes!

Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

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

I made a couple of small fixes. This looks ready to merge once CI passes.

@max-sixty
Copy link
Contributor Author

Thanks @jturner314 !

@max-sixty
Copy link
Contributor Author

Merge conflicts resolved

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

Successfully merging this pull request may close these issues.

3 participants