Closed PoslinskiNet closed 8 years ago
Thanks for this. It's a handy change. You need to add specific tests to test this scenario though. Also, rather than comment out code, just remove it and explain why.
@brendon I've applied your feedback. Hopefully, I will find some time to add specs later this week.
Thanks @PoslinskiNet, quite a few existing tests are failing now so you'll need to address why that is. I suspect it's because you've changed some pretty core methods. You may want to run the test suite locally to ensure things aren't broken before pushing your changes to the PR. Just run rake
to run the tests once you've bundled.
@brendon I made some improvement, so most of the test are passing. However, 3 of them are still failing, so probably I will fix them later on.
Thanks @PoslinskiNet, remember to also add those tests to test the functionality you're trying to add. :)
👍 would be nice to have this fix in next release
Hi @jpalumickas, thanks for the spec to test your new feature. Unfortunately we can't accept this PR until the entire test suite is green. Can you work on this when you have time? This is a great feature to support, so I look forward to seeing it merged :)
@brendon @jpalumickas I've added some specs, but some other are still failing, and I don't have time to investigate it right now. If anyone sees the problem, I would be really grateful for the fix :).
@PoslinskiNet this is because you changed order in higher_items
. You need to update 3 failing specs to use updated order.
Tasks:
[15, 25, 55]
.