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

Add into_scalar for ArrayView0 and ArrayViewMut0 #700

Merged
merged 7 commits into from Sep 8, 2019

Conversation

LukeMathWalker
Copy link
Member

This addresses #688.

I have taken the chance to re-organize impl_views, converting it into a folder sub-module to then split it into smaller files with a functionality-related name.
I think this makes it easier to navigate the crate (and we should probably do the same for other very big files we have in the repository) but I am ok to revert the changes if you believe otherwise.

/// let scalar: &Foo = view.into_scalar();
/// assert_eq!(scalar, &Foo);
/// ```
pub fn into_scalar(self) -> &'a A {
Copy link
Member

@bluss bluss Sep 8, 2019

Choose a reason for hiding this comment

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

the ArrayView doesn't need to be consumed to allow this (infuriating difference between shared and mut for IndexLonger). I guess one could just as well make this a &self method, it makes a difference for non-copy ArrayViews?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but then I opted to keep the signature consistent with the other two (Array/ArrayViewMut). Given that it's named into_*, it sounded preferable to consume the input.
On the other hand, taking &self makes it more convenient for ArrayView because you can keep using self afterwards.

I don't know, I would lean on the side of symmetry in this case.

Copy link
Member

Choose a reason for hiding this comment

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

ok, that works.

@bluss
Copy link
Member

bluss commented Sep 8, 2019

Looks good. Continual splitting is good I think. Impl_views isn't even large in ndarray 😄

@LukeMathWalker LukeMathWalker merged commit e4d2711 into rust-ndarray:master Sep 8, 2019
@LukeMathWalker LukeMathWalker deleted the into-scalar branch September 8, 2019 17:19
@bluss
Copy link
Member

bluss commented Sep 8, 2019

Tip, use the https://help.github.com/en/articles/closing-issues-using-keywords names. With "Fixes #688." it closes the issue automatically. I prefer to put these in the PR text (not in the commit log because rebasing and re-pushing causes spam on the issue).

@LukeMathWalker
Copy link
Member Author

Nifty trick, thanks!

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.

None yet

2 participants