search

ckdake / setler

Setler lets you use the 'Feature Flags' pattern or add settings to models in Rails
MIT License
84 stars 21 forks source link

WIP: Rails 4 compatibility #25

Closed iamvery closed 11 years ago

iamvery commented 11 years ago

Here's a first try at setler compatibility with Rails 4. There is a pretty big snag, and I'm not sure how we'll want to approach it. This patch, as is, would be API incompatible with previous versions of setler :frowning:

The gist of the issue is with Rails 4's change to all it is now used as a base for other methods such as where. Since setler overrides all there is actually an infinite loop introduced when using setler with Rails 4.

The second problem is the lack of attr_accessible in Rails 4. This was easily addressed by conditionally calling attr_accessible based on the Rails' major version rather than the ability to respond_to the method.

What do you think? It doesn't quite feel done, but I'm not sure what the right answer is.

ckdake commented 11 years ago

Nice! Changing the API is :cry: but if it has to be done it can be done. Maybe there is a way to do some terrible code duplication that works but shows a deprecation notice in <4 and does the new api in 4+?

It'd be great for you to talk to @alindeman about this too.

iamvery commented 11 years ago

I like the sound of that! I'll see if I can put something together.

iamvery commented 11 years ago

I tried using ActiveSupport::Deprecation instead of Kernel#warn, but it wasn't working for some reason. Not sure about best practice here.

ckdake commented 11 years ago

Any other changes needed for Rails 4 or is this it? I'm :thumbsup: for merging if it's enough!

iamvery commented 11 years ago

All the tests (that were passing with Rails 3, see below) are passing with Rails 4 here. There are a number of deprecation warnings that should probably be addressed, but it is confirmed working with Rails 4.

Note: The last test has always failed for me despite Rails version.

ckdake commented 11 years ago

v0.0.12 released!