search

pat / combustion

Simple, elegant testing for Rails Engines
MIT License
707 stars 51 forks source link

Add formal version comparison helper. #126

Closed pat closed 2 years ago

pat commented 2 years ago

Prompted by discussion in #123 and #125, though I realise this steps on @jrochkind's kind efforts, sorry! 😅

I considered whether detecting the presence of methods would be the best way forward - but given the code is extensively checking Rails/ActiveRecord versions, I opted to lean into that but with a neater syntax to leverage Rubygems/Bundler's version constraint logic rather than string-to-float wrangling. Also, it refers directly to the locked gem versions via Bundler - not something I've done before, but it seems to work well.

pat commented 2 years ago

Updated the commit to give credit also to @jrochkind, feels more than fair given your efforts around this!

jrochkind commented 2 years ago

My PR that I can't manage to get to pass? Ha, please, step on my toes! :)

jrochkind commented 2 years ago

The way I've loaded "currently loaded version of the gem" before is: 

Gem.loaded_specs["some_gem_name"].version

I guess it doesn't surprise me that Bundler and Rubygems both offer their version of getting the version of the currently loaded gem by name. Whatever works, it seems like you've found some more convenient alternate API to what I knew, with the matches API and such, cool.

pat commented 2 years ago

Thanks for all the feedback, greatly appreciated!

I've renamed the gate instances, plus I've decided to use Gem.loaded_specs (which I wasn't aware of) instead of Bundler - even though it returns Bundler-wrapped specs. This means we lean on just rubygems (directly), rather than both rubygems and bundler. Even if I can't think of a realistic situation where a Rails engine won't have Bundler present, I like the idea of simplifying dependency interactions.

Also, put a guard clause in to handle situations where spec could be nil - though again, this feels like an unlikely situation (the ActiveRecord-specific clause should only come into play when AR is present).