search

activestylus / padrino-fields

Simple, flexible form helpers for the Padrino Framework
MIT License
22 stars 2 forks source link

Added ActiveRecord and improved styling hooks #3

Closed whitequark closed 11 years ago

activestylus commented 12 years ago

Looks interesting. I like the idea of making it bootstrap friendly.

However you are missing tests for ORM integration, and a few methods as well. Have a look at the datamapper spec

https://github.com/activestylus/padrino-fields/blob/master/test/test_datamapper.rb

And the ORM Class

https://github.com/activestylus/padrino-fields/blob/master/lib/padrino-fields/orms/datamapper.rb

whitequark commented 12 years ago

Yeah, I forgot about tests. I'll look into adding them.

Bootstrap friendliness is a bit hacky (as is the padrino-fields overall), but it works as a first approximation. I'd say that the whole library needs some rework and more consistency in the configuration. Maybe I'll take this task.

As per ORM methods, I don't quite understand the purpose of form_reflection_validators, and even worse, all the methods I did not implement are not used anywhere in the codebase. Are they really needed?

Also I'm quite sure that AR and DM validators have incompatible interfaces, so returning a validator object itself (or whatever the reflection API provides for a particular ORM) won't be very useful.

activestylus commented 12 years ago

They are undocumented for now but the tests are all passing and show pretty clear intent form_reflection_validators will return validations for a specified child model. form_attribute_validators will grab all validations for a model. I have found this useful for customizing error messages. However I can trim it out of core if no one's using it. I'm open to hearing any possible improvements

whitequark commented 12 years ago

I was't opposed to them, I just didn't seem any reason to implement a method which wasn't used. I think they well belong to core.

I'm not sure about any particular format, but could you take a look at validation-reflection gem? I have never used DataMapper, or, for that record, any ORM except AR.

gambhiro commented 11 years ago

Sounds great, is something missing for the merge?

activestylus commented 11 years ago

I was a bit off the grid for a minute there. This looks great though. Merging right now

Thanks!