search

ruckus / quickbooks-ruby

Quickbooks Online REST API V3 - Ruby
MIT License
374 stars 302 forks source link

Loosen roxml version constraint #524

Closed rmacklin closed 4 years ago

rmacklin commented 4 years ago

Prior to this PR, the quickbooks-ruby gem has declared a version constraint for the roxml gem of exactly '4.0.0'. This means that applications using quickbooks-ruby are unable to use a later version of roxml (such as the recently released roxml v4.1.0).

To fix this problem, I've loosened the roxml version constraint to '~> 4.0', which makes it compatible with any minor versions of 4.x.

I didn't choose '>= 4.0' because it's possible that roxml would release a 5.x version with a breaking change that would not be compatible with quickbooks-ruby. But as long as roxml follows semver, it should be safe to change this to '~> 4.0'.

rmacklin commented 4 years ago

I'm not sure why the CI checks aren't showing up on this PR, but Travis ran and passed: https://travis-ci.org/github/ruckus/quickbooks-ruby/builds/697066183

image

rmacklin commented 4 years ago

@ruckus Let me know if I missed something important about why this had to be locked down to exactly 4.0.0

FWIW, the only change in the library code for roxml v4.1.0 is the BigDecimal deprecation fix: https://github.com/Empact/roxml/compare/v4.0.0...v4.1.0

ruckus commented 4 years ago

@rmacklin thank you so much for this, its great.

Prior to this PR, the quickbooks-ruby gem has declared a version constraint for the roxml gem of exactly '4.0.0'

I had done this because in my own Rails app I am super-defensive about locking to specific and known versions. I've been bit in the ass by being too loose and pulling in an update which caused regressions.

But I agree that '~> 4.0' should be OK.

rmacklin commented 4 years ago

@rmacklin thank you so much for this, its great.

Glad to help! Thank you for the quick review :)

Any chance you'd be able to publish a new release with this change? That would allow our team to finally upgrade our application to the official versions of quickbooks-ruby and roxml!

rmacklin commented 4 years ago

Prior to this PR, the quickbooks-ruby gem has declared a version constraint for the roxml gem of exactly '4.0.0'

I had done this because in my own Rails app I am super-defensive about locking to specific and known versions. I've been bit in the ass by being too loose and pulling in an update which caused regressions.

I agree that it's a best practice to be defensive and lock to known versions in applications to avoid being bitten by an update that causes regressions. But for gems I think it's best to loosen constraints to at least a semver minor version. (Otherwise you can run into the situation that prompted this PR: users of the gem are unable to upgrade another gem even though the new version is compatible.) When gems declare looser constraints, it puts the application maintainers in control: the onus is on them to be strict about locking down to known-good versions, but they are free to try upgrading on their own schedule.

Anyway, thanks again for the prompt response!

ruckus commented 4 years ago

Yep! I just pushed 1.0.10 with your PR merged. Thanks again!!!

rmacklin commented 4 years ago

Thanks so much! I can see https://github.com/ruckus/quickbooks-ruby/commit/25400e1982953da9bef55a5b3577ad74ab2da6d2 has been tagged with 1.0.10 although at the time I'm writing this, master is still pointing at https://github.com/ruckus/quickbooks-ruby/commit/d480a70288f2380bb5574cbf81e49bc356b6e308:

image

Should we fast-forward master to 25400e1982953da9bef55a5b3577ad74ab2da6d2?

ruckus commented 4 years ago

Doh, I always forget the final push. master is updated now