Fixed memoization of RankedModel::Ranker::Mapper#finder #120
Conversation
It now caches a value for each passed sort order; without this multiple calls to finder can cause weird flipping issues where the whole list gets reversed.
|
The automated test suite is a bit out of date (and thus failing to install in some instances). I've successfully run the tests locally with the |
|
FWIW, @brendon has my full endorsement, I am thankful for all the great work he's done on |
|
@mixonic, what are the chances of getting this merged? If I put in the work to get the test suite installing correctly, will you have the time to merge? |
|
@brendon yes of course, and I'm happy to add you as a maintainer as well. I'd very much like to minimize merging failing builds, or at least only merge them if we have a documented plan to resolve the issues quickly. In the supported Travis matrix only builds of 1.9.3 and rbx are failing. |
|
For Ruby 1.9.3 it looks like we need to pin the version of
|
|
Thanks @mixonic, I've used Appraisal in the past to better facilitate the travis test matrix. I'm attempting to convert |
|
Lol, that's the first conversion I've done that's been green first go! :D https://travis-ci.org/mixonic/ranked-model/builds/402557044 I excluded rubinius and jruby. I'll now add them back in. Depending on the results, perhaps we should exclude rubinius? |
|
Looks like we have a flappy test: https://travis-ci.org/mixonic/ranked-model/jobs/402589391#L1133 For now I guess let's ignore that and get this PR merged. I'm close to getting it green now for all rubies. Just waiting on a green for rbx and jruby, then will merge them all together and do one last run. Hopefully our flappy test doesn't rear its head. |
|
Looking a bit closer at the test, could you explain it for me? I've open a seperate issue: #126 |
|
Ok, I've enabled all the combinations now. I'm off to bed so I'll see how this ran in the morning. If you could give some insight into that randomly failing test that would be helpful. Seems to only occur on jruby and rbx. Could be a threading issue? |
|
Hi @mixonic :) Great to see the matrix is green! I think merge this, then we can look at why that test is occasionally failing. We can address it in another PR. And yes please, if you can make me a maintainer that would be great. I'll then try to get some action on the other PR's and issues waiting in the wings. |
Having updated some ducks with age and pond details, we can no longer rely on their retrival order (at least with Postgresql). We should instead target the pond and rank by age so we know we're getting the first two ducks in the pond. This doesn't affect the idea of the test which is to make sure that ducks added to the end of the list via the maximum value are indeed added to the end of the list.
|
@mixonic, can you force a re-run of the suite? One of the VM's stalled: https://travis-ci.org/mixonic/ranked-model/jobs/403051387#L447 |
| - 2.2.10 | ||
| - 2.3.7 | ||
| - 2.4.4 | ||
| - 2.5.1 |
There was a problem hiding this comment.
Fantastic! Thank you for updating this.
| @duck_11 = Duck.offset(10).first | ||
| @duck_12 = Duck.offset(11).first | ||
| @duck_11 = Duck.where(:pond => 'Pond 1').rank(:age).first | ||
| @duck_12 = Duck.where(:pond => 'Pond 1').rank(:age).second |
There was a problem hiding this comment.
Seems ok. I'm very curious why offset isn't sufficient.
|
Thanks @mixonic! :) Great to have this merged :D My next job will be looking at Rails 5.x support. I don't think there's too much to do (some stuff around Thanks again! |
|
landed in v0.4.1 |
It now caches a value for each passed sort order; without this multiple calls to finder can cause weird flipping issues where the whole list gets reversed.
NOTE: @mixonic, this is basically the same as #110 but with an associated test that fails without the fix. The test came from here: 37e19f8
Credit to @jmchambers and @mpokrywka for the actual work.
Closes #103
Also makes #110 redundant.