DmitryTsepelev / ar_lazy_preload

Lazy loading associations for the ActiveRecord models
MIT License
677 stars 24 forks source link

Don't do merge if there is nothing to merge #33

Closed Earendil95 closed 4 years ago

Earendil95 commented 4 years ago

Hey Dmitry!

Thanks for this gem :)

I've noticed one minor possible improvement. In ArLazyPreload::Merger before choosing a merging strategy, we check if we need to merge something. The thing is: other.lazy_preload_values is always true when it comes to if other.lazy_preload_values. This unexpected merging provoked some errors, pretty similar to one discussed in #30. Of course, this PR is not going to fix the real issue (possible dead associations) but it can prevent people from catching weird errors right after including ar_lazy_preload to Gemfile from unexpected places and blaming ar_lazy_preload (as it appears in the very top of backtrace).

PS. I didn't find a way to test this improvement. If you have ideas about it, please, share them and I will add a test here.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 243


Totals Coverage Status
Change from base Build 241: 0.0%
Covered Lines: 647
Relevant Lines: 679

💛 - Coveralls
PikachuEXE commented 4 years ago

I think you can post the benchmark like I did in #16

Earendil95 commented 4 years ago

@PikachuEXE thanks for the comment

To be honest, I don't think a benchmark is valuable here because no big optimizations have been done here, only error-proof.

DmitryTsepelev commented 4 years ago

Agreed, it won't affect benchmarks, but the check definitely makes sense. Thank you so much!

Earendil95 commented 4 years ago

@DmitryTsepelev thanks a lot for your quick reaction!