franckverrot / activevalidators

Collection of ActiveModel/ActiveRecord validators
https://rubygems.org/gems/activevalidators
MIT License
306 stars 49 forks source link

date validator require not working - bounces back #71

Closed Incanus3 closed 10 years ago

Incanus3 commented 10 years ago

Hi, when I call ActiveValidators.activate(:date), it loads 'activevalidators/lib/active_model/validations/date_validator', which requires 'date_validator' (the gem), which requires 'date_validator/lib/date_validator', which then requires 'active_model/validations/date_validator'. This should load the file from date_validator gem, but it actually tries to load it from activevalidators gem (which was loaded in the first place), so this is a noop and the ActiveModel::Validations::DateValidator isn't defined.

This means date_validator stops working when activevalidators is higher in the $LOAD_PATH. This could be solved if date_validator/lib/active_model was moved to date_validator/lib/date_validator/active_model and required with the date_validator prefix, but this means changing the depended upon gem, which I guess is a no go. Is there any other way around this problem?

Incanus3 commented 10 years ago

putting this before the call to ActiveValidators.activate fixed the problem for me, but it's kinda ugly:

date_validator_path = $:.find {|path| path =~ /date_validator/}
$:.delete date_validator_path
activevalidators_position = $:.index {|path| path =~ /activevalidators/}
$:.insert(activevalidators_position, date_validator_path)
franckverrot commented 10 years ago

Thanks for that thorough bug report.

I wouldn't like to manager the LOAD_PATH manually so I'll find another way of doing this. Please feel free to propose any change, I'll take care of this in a couple of days.

Thanks again!

Incanus3 commented 10 years ago

Glad to be of some use. This is a great gem (which I've found way too late), so it would be unfortunate if such a core feature misbehaved. Thanks for your hard work.

franckverrot commented 10 years ago

Hey thanks :-)

That "core feature" is pretty new. I wanted to avoid loading all the gems (like AV 2.0 was doing) in this new version... seems like it's not working a 100% the way I wanted it to... :tired_face:

Incanus3 commented 10 years ago

Yep, conditional loading is a nice thing, when the dependencies start to crop up. As I'm thinking about it, you cound actually move the lib/active_model dir to lib/active_validators/active_model and load it with the active_validators prefix, this should solve the clash with date_validator, if I'm not missing something. Also, looking at the top-level lib/activevalidators.rb file, why are the phony and countries gems loaded in there and not in the validators which require them?

franckverrot commented 10 years ago

Sorry I didn't got back at you earlier.

I'm releasing a fix in a couple of hours. Thanks a lot for the detailed analysis. I actually went through the idea of adding this "extra-layer" a couple of times in the past and never got any urge of doing so... This was the reason why this should have been done earlier :+1:

Thanks again!