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 a free sorted_unstable #796

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

chris-ha458
Copy link
Contributor

Would it be possible to merge this?

- based on free `sorted`
@Philippe-Cholet
Copy link
Member

It surely seems legitimate. But I don't know what is our policy about "free" functions yet.
@phimuemue @jswrenn

Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

@chris-ha458, can you say a little bit about why you need this to be a free function? Why is the existing method not adequate?

@chris-ha458
Copy link
Contributor Author

It definitely is not a "need" but a "nice to have".

I'm not finding the specific snippet of code right now but it looks a bit like this.

use itertools::sorted;

fn process_vec(mut input: Vec<String>) -> Vec<String> {
    input.push("1".to_string());
    // other processing steps
    sorted(input)
}

This is a small contrived example, but i think you can see how it could be useful.
Right now if I want to use unstable sort, it would have to end with

input.sort_unstable()
input
}

With my PR it could follow the above

sort_unstable(input)
}

This proposed change is minor and not critical. However, it could enhance the functionality in scenarios involving functional chaining, tail calls, maps, and closures.

While introducing additional code increases maintenance complexity, only looking at the immediately related sections of the code, the simplicity of this change does not seem to significantly add to that burden.

I understand that this PR may be closed for various reasons. Nonetheless, explanations for closing or prerequisites for merging would be beneficial, if only for documentation purposes.

@scottmcm
Copy link
Contributor

scottmcm commented Nov 7, 2023

I think the note is that you could use https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.sorted_unstable to write

fn process_vec(mut input: Vec<String>) -> Vec<String> {
    input.push("1".to_string());
    // other processing steps
    input.into_iter().sorted_unstable()
}

without a free function, while still moving it.

But it's certainly true that if https://docs.rs/itertools/latest/itertools/fn.sorted.html was apparently decided to be worth having at some point, it's hard to say why a sorted_unstable function wouldn't be...

@chris-ha458
Copy link
Contributor Author

I agree with all of your points.
Coming from developing with python, my intuition was that sorted only exists to provide a similar interface in rust. Tbh I'm not sure that would have been a good idea, since there are different idiomatic patterns are available in rust, but such design choices happen all the time.

If that were the case it would be easy to reject this one. First of all, python does not offer a sorted_unstable function. Secondly offering sorted in the first place would have been a suboptimal choice and this would be doubling down on a bad path.

I don't think this is a problem, hence authoring the PR but I could definitely understand anybody that does..

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Nov 7, 2023

It's either input.into_iter().sorted_unstable() or (the proposed) sorted_unstable(input). Free functions convert into an iterator under the hood, which for me is the main selling point.
#91 added the free "sorted" (I was looking for a reason that would help us here but no).

Edit: About free functions, I found that interesting.

@jswrenn jswrenn added this pull request to the merge queue Nov 13, 2023
@jswrenn jswrenn added this to the next milestone Nov 13, 2023
Merged via the queue into rust-itertools:master with commit 9c302ce Nov 13, 2023
8 checks passed
@chris-ha458 chris-ha458 deleted the sorted_unstable branch November 14, 2023 05:47
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
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.

4 participants