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 ability to sort mixed-type lists to JsonSlurper #58

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

cdeszaq
Copy link
Collaborator

@cdeszaq cdeszaq commented Oct 6, 2016

Before, if a list started with a Comparable (ie. string, boolean, or number), it would be sorted using the objects' comparability. This broke if the list contained more than 1 type, like a String and a Map. With this change, that is fixed, and it should retain any sorts that existed before.

Note: Only impacts test code

@@ -66,6 +66,8 @@ Current

- [#45, removing sorting from weight check queries](https://github.com/yahoo/fili/pull/46)

- `JsonSlurper` can now handle sorting lists with mixed-type entries, even if the list starts with a string, number, or
boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a link to the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -5,6 +5,7 @@
/*
* Changed HashMap to LinkedHashMap for consistency. Added ability to optionally sort the lists and maps.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

This code was borrowed from Groovy right? While we're here, we really should update the copyright notification to reflect that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@michael-mclawhorn
Copy link
Contributor

michael-mclawhorn commented Oct 7, 2016

👍 (Once Andrew is satisfied)

@codingwhatever
Copy link
Collaborator

👍

Also corrected licensing for JsonSlurper
@cdeszaq cdeszaq merged commit 7052d0a into master Oct 10, 2016
@cdeszaq cdeszaq deleted the fixJsonSlurpSort branch October 10, 2016 17:54
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.

4 participants