Skip to content

Adds Stream:ge #285

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

Merged
6 commits merged into from
Oct 15, 2019
Merged

Adds Stream:ge #285

6 commits merged into from
Oct 15, 2019

Conversation

assemblaj
Copy link
Contributor

@assemblaj assemblaj commented Oct 7, 2019

let result_greater_vals = vec![1., 2., 4.].into_iter().collect::<VecDeque<f64>>()
.partial_cmp(vec![1., 2., 3.].into_iter().collect::<VecDeque<f64>>()).await;
let result_none = vec![std::f64::NAN].into_iter().collect::<VecDeque<f64>>()
.partial_cmp(vec![1.].into_iter().collect::<VecDeque<f64>>()).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is quite the comparison haha. I'm wondering if we could perhaps move most of this to a dedicated test under src/stream/stream/partial_cmp.rs, and then add a more reduced example as part of the doc comment. That way we keep the thoroughness of the tests to catch regressions while gaining legibility.

I'm afraid that in its current form folks might be somewhat overwhelmed by the documentation, which is something we generally want to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR was branched from assemblaj-partial_cmp, that's the example code for partial_cmp. I can go back and update the partial_cmp example it to be cleaner and then update this branch.

The issue I was having is that the std::iter example code for partial_cmp just creates and compares the iterators inline; doing this with VecDeque's doesn't look as clean but I'm not sure about making a few at the top of the function and copying them everywhere either (this is what I did for the Stream::ge examples, see below). How do you think I should approach this?

let single:     VecDeque<isize> = vec![1].into_iter().collect();
let single_gt:  VecDeque<isize> = vec![10].into_iter().collect();
let multi:      VecDeque<isize> = vec![1,2].into_iter().collect();
let multi_gt:   VecDeque<isize> = vec![1,5].into_iter().collect();

assert_eq!(single.clone().ge(single.clone()).await, true);
assert_eq!(single_gt.clone().ge(single.clone()).await, true);

assert_eq!(multi.clone().ge(single_gt.clone()).await, false);
assert_eq!(multi_gt.clone().ge(multi.clone()).await, true);

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Overall this patch looks really good! -- left a small nit, but I think we should be good to merge once that's addressed!

edit: this PR also needs to be rebased

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Oct 8, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks!

@ghost ghost merged commit a7041be into async-rs:master Oct 15, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants