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 in place sorting of OrderMap and OrderSet #55

Closed
vitiral opened this issue Jan 2, 2018 · 10 comments
Closed

Implement in place sorting of OrderMap and OrderSet #55

vitiral opened this issue Jan 2, 2018 · 10 comments

Comments

@vitiral
Copy link

vitiral commented Jan 2, 2018

Feature request: Add in-place sorting methods to OrderMap (and OrderSet). Similar to the existing .sorted_by, these methods can allow the user to sort by either keys or values or both by supplying all of those to the closure.

For example:

fn sort_by<F>(&mut self, cmp: F)
    where F: FnMut(&K, &V, &K, &V) -> Ordering;

Original issue:


Feature request: add sort_keys<K:Ord+Hash>(&mut self) to do an inplace sort.

This library is INCREDIBLE when assertions need to be made because I can do an inplace sort and the diffs look good (when combined with pretty_assertions of course).

I did a naive implementation like this:

pub fn sort_ordermap<K:Ord+Hash, V>(m: &mut OrderMap<K, V>) {
    let mut ordered: Vec<_> = m.drain(..).collect();
    ordered.sort_by(|left, right| left.0.cmp(&right.0));
    m.extend(ordered.drain(..));
}

pub fn sort_orderset<K:Ord+Hash>(m: &mut OrderSet<K>) {
    let mut ordered: Vec<_> = m.drain(..).collect();
    ordered.sort_by(|left, right| left.cmp(&right));
    m.extend(ordered.drain(..));
}

I'm sure a better one can be done with full access to the underlying data structure.

@vitiral vitiral changed the title implement sort_keys() for OrderMap+Set implement sort_keys() for OrderMap and sort() for OrderSet Jan 2, 2018
@vitiral vitiral changed the title implement sort_keys() for OrderMap and sort() for OrderSet implement sort_keys() for OrderMap and sort() for OrderSet Jan 2, 2018
@clarfonthey
Copy link

You could also use sort_by_key(|elem| elem.0) for the map and just sort for the orderset.

@vitiral
Copy link
Author

vitiral commented Jan 2, 2018

@clarcharr do you mean it could be named that? I suggested sort for OrderSet in the title of the issue. I'm not sure what sort_by_key means -- you could implement sort_by in addition to sort_keys -- both could be useful!

@clarfonthey
Copy link

Sorry; to clarify, you could replace the code in the post with:

pub fn sort_ordermap<K:Ord+Hash, V>(m: &mut OrderMap<K, V>) {
    let mut ordered: Vec<_> = m.drain(..).collect();
    ordered.sort_by_key(|x| x.0);
    m.extend(ordered);
}

pub fn sort_orderset<K:Ord+Hash>(m: &mut OrderSet<K>) {
    let mut ordered: Vec<_> = m.drain(..).collect();
    ordered.sort();
    m.extend(ordered);
}

@vitiral
Copy link
Author

vitiral commented Jan 2, 2018

Oh, I didn't realize that sort_by_key was a thing! Thanks!

@bluss
Copy link
Member

bluss commented Jan 2, 2018

I agree with this feature request. No comment on whether it is possible easily

@bluss
Copy link
Member

bluss commented Jan 2, 2018

A side note about sort_by_key, is that the key version sometimes will need to clone the keys when the _by version does not.

@bluss
Copy link
Member

bluss commented Jan 3, 2018

I have taken the liberty to edit the original post to better reflect what I think we are seeking. It's your post, so you are free to edit it too, it's a collaboration..

@bluss bluss changed the title implement sort_keys() for OrderMap and sort() for OrderSet Implement in place sorting of OrderMap and OrderSet Jan 3, 2018
@vitiral
Copy link
Author

vitiral commented Jan 3, 2018

sort_by would be a good first start. Additional methods (like sort_keys) can be added later at other's requests.

@bluss
Copy link
Member

bluss commented Jan 3, 2018

Implemented as an experiment in #57, (building upon the retain in place PR, so that one needs to merge first).

Yes, instead of Vec's .sort() ordermap should probably have .sort_keys(), that sounds nice

@bluss
Copy link
Member

bluss commented Jan 5, 2018

Fixed in #57

@bluss bluss closed this as completed Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants