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

Don't depend on unordered object #393

Merged
merged 2 commits into from
Dec 27, 2014
Merged

Conversation

falsefalse
Copy link
Contributor

Before, passing range as {min: 1, max: 5, 25%: 2, 50%: 3} and {min: 1, 25%: 2, 50%: 3, max: 5} would mean different results.
Because objects are not guaranteed to return keys in same order, 2 resulting number arrays need to be sorted.

Before, passing `range` as `{min: 1, max: 5, 25%: 2, 50%: 3}` and `{min: 1, 25%: 2, 50%: 3, max: 5}` would mean different results.
Because objects are not guaranteed to return keys in same order, 2 resulting number arrays need to be sorted.
@falsefalse
Copy link
Contributor Author

There are also this.xSteps and this.xNumSteps arrays, I'm not sure if they need sorting as well.

Also please let me know if you want me to add tests for this.

@falsefalse
Copy link
Contributor Author

This is also similar to #323

@leongersen
Copy link
Owner

The other arrays would need sorting too, as they are referenced by the same indexes as the range. Im fairly sure the current tests will guarantee the normal behavior, but tests demonstrating the ability to provide unsorted arrays would be nice.

I'm working to get some other fixes merged, so this could ship before the new year.

@falsefalse
Copy link
Contributor Author

How do I approach sorting the other two, then? They contain mixed type values, numeric sort won't do as easily. What are they for?

@leongersen
Copy link
Owner

xSteps holds the percentage-based size of the step for the current sub-range, relative to the sub-range. xNumSteps contains the numeric value for those steps. The value will be false if there is no step for the sub-range. The sort has to be identical to the sorting of the range, so that index n in the range array would match the same n in the steps array, before and after sorting.

Turn range into array, sort it by value and use then.
This makes sure that all 4 arrays are naturally ordered the same way.
@falsefalse
Copy link
Contributor Author

It passes tests

@leongersen leongersen modified the milestone: 7.0.10 Dec 27, 2014
@leongersen leongersen merged commit 6ce02c7 into leongersen:master Dec 27, 2014
@leongersen
Copy link
Owner

This is in the newest release: 7.0.10. Thanks a lot for your contributions!

@falsefalse
Copy link
Contributor Author

Awesome!

And thanks a lot for your prompt replies :)
On 27 Dec 2014 16:08, "Leon Gersen" notifications@github.com wrote:

This is in the newest release: 7.0.10. Thanks a lot for your contributions!


Reply to this email directly or view it on GitHub
#393 (comment).

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.

2 participants