ruckus / quickeebooks

ABANDONED !!!! Quickbooks + Ruby using the Quickbooks Online v2 Data Services REST API
MIT License
79 stars 67 forks source link

Modify ruby version support #109

Closed mcmire closed 10 years ago

mcmire commented 10 years ago

This is a bit of a controversial PR I realize, so I'd like to open a discussion around this.

I basically explain the rationale behind this PR in the commit comments below but here they are again:

The gemspec requires activemodel, but does not specify a version
dependency. However, Rails 4 now requires >= 1.9.3. This means that it
is impossible to run `bundle` on 1.9.2 and REE, which means it's
impossible to develop on those versions. This is also causing tests to
not run on Travis.

Unfortunately, there isn't a good way to fix this in the gemspec, since
it's a bad idea to have conditional dependencies. While I'm sure some
people are still on 1.9.2, it is obsolete and unsupported. So the best
way to solve this is just to drop support.

This means we also no longer need to use custom Gemfiles on Travis.
These Gemfiles ensured that anything before 1.9.3 set the ActiveModel
dependency to 3.x and anything after was using 4.x. This is false
because this is inconsistent with what the gemspec is saying, which is
that quickeebooks depends on *any* version of ActiveModel (meaning that
people who install this gem on <= 1.9.3 will actually always get
ActiveModel 4, not 3).
ruckus commented 10 years ago

Yeah I hear. Its too bad we cannot get any usage metrics on which Ruby versions people are using.

On one hand I only use the Validations module in ActiveModel - which seems like pulling down the whole truck just for the shifter knob. I'd like to ultimately implement rip out ActiveModel in favor of just a lean Validations package and then we can skirt this issue as well.

Can you recommend any Validations packages that can be swapped in?

mcmire commented 10 years ago

At one point Validatable existed -- it's basically a small clone of the way validations worked in Rails 3. It was used for a while in MongoMapper but they have since switched over to ActiveModel::Validations. So it's basically abandoned now (there are several forks of it), so I don't know if it would be a good idea to use, but that's an option.

There's also Verity which takes a more RSpec-y approach to validations. While not the approach I would have taken, the library seems small and flexible. But again, it hasn't been maintained in a while. Also, I don't know if it's used anywhere.

Finally there's Scrivener which is used internally by Ohm, a small ORM for Redis. It is very small and takes a magic-less approach where you make a #validate method and declare your validations at runtime. Note that they intend you use Scrivener by storing validations outside the model (as in, more of a view object), but since there aren't any views here, you can easily mix in validations by includeing Scrivener::Validations. Again, though, this does not seem to be maintained anymore (I know because I used this on a project a year ago and we tried submitting a PR and as of yet it still hasn't been pulled in).

So, it seems that everyone either uses ActiveModel::Validations or just writes their own library. I hate to re-invent the wheel, though -- it's unfortunate that these libraries have fallen out of maintenance and it's inevitable that you'll have to fork one of them at some point, but I guess that's how it works. Anyway, if you want a drop-in replacement, then Validatable seems like it would work. Otherwise I would recommend Scrivener as it seems pretty palatable and easy to modify.

ruckus commented 10 years ago

Alright, so it sounds like sticking with ActiveModel is the safest route.

Which then means that merging in your PR is safe. Is that correct?

On Oct 19, 2013, at 11:27 AM, Elliot Winkler notifications@github.com wrote:

At one point Validatable existed -- it's basically a small clone of the way validations worked in Rails 3. It was used for a while in MongoMapper but they have since switched over to ActiveModel::Validations. So it's basically abandoned now (there are several forks of it), so I don't know if it would be a good idea to use, but that's an option.

There's also Verity which takes a more RSpec-y approach to validations. While not the approach I would have taken, the library seems small and flexible. But again, it hasn't been maintained in a while. Also, I don't know if it's used anywhere.

Finally there's Scrivener which is used internally by Ohm, a small ORM for Redis. It is very small and takes a magic-less approach where you make a #validate method and declare your validations at runtime. Note that they intend you use Scrivener by storing validations outside the model (as in, more of a view object), but since there aren't any views here, you can easily mix in validations by includeing Scrivener::Validations. Again, though, this does not seem to be maintained anymore (I know because I used this on a project a year ago and we tried submitting a PR and as of yet it still hasn't been pulled in).

So, it seems that everyone either uses ActiveModel::Validations or just writes their own library. I hate to re-invent the wheel, though -- it's unfortunate that these libraries have fallen out of maintenance and it's inevitable that you'll have to fork one of them at some point, but I guess that's how it works. Anyway, if you want a drop-in replacement, then Validatable seems like it would work. Otherwise I would recommend Scrivener as it seems pretty palatable and easy to modify.

— Reply to this email directly or view it on GitHub.

mcmire commented 10 years ago

I wasn't sure if you were comfortable with dropping support for 1.9.2, so if you're fine with that, then yes it's safe to merge this.

ruckus commented 10 years ago

I'm uncomfortable dropping support if there is still a non-trivial number of users on 1.9.2.

From my understanding since ActiveModel now requires >= 1.9.3 and we use ActiveModel - they have basically made the choice for us.

I guess this would punish Rails 3.x users on Ruby 1.9.2? Any ideas on number of users on those platform versions?

On Oct 22, 2013, at 11:38 AM, Elliot Winkler notifications@github.com wrote:

I wasn't sure if you were comfortable with dropping support for 1.9.2, so if you're fine with that, then yes it's safe to merge this.

— Reply to this email directly or view it on GitHub.

mcmire commented 10 years ago

Okay, that's fair. Now that I think about it I'm sure people still use 1.9.2, so it is probably unwise to drop support.

Taking a step back... I just realized we use the same approach you did in our gems. For instance, shoulda-matchers is designed to work with both Rails 3 and 4. We declare a dependency on ActiveSupport >= 3 in the gemspec, but then for tests, we use Gemfiles just like you did which switch the version to either 3 or 4. This means, technically, on 1.9.2, we would run into the same problem as I did here, where running bundle would fail. I think the way we "get around" it is that we don't develop on 1.9.2, so I guess it isn't a big deal to anyone.

So I think your approach is actually fine, but I would still like to think if I have 1.9.2 I am able to run tests. What if you were to keep the activemodel dependency in the gemspec but then add logic to the Gemfile so that if I'm on Ruby >= 1.9.3, I get Rails 4 by default, otherwise I get Rails 3? (I actually don't know if Bundler would complain or not due to duplicate activemodel statements, but perhaps it is worth trying and seeing what happens.)

mcmire commented 10 years ago

Okay, this PR has gotten a little out of focus -- I'm going to go ahead and close it and open another one that just focuses on fixing the tests so they run on 1.9.2 (and REE).