Added ActiveRecord and improved styling hooks #3

Merged
merged 4 commits into from Sep 8, 2013

Conversation

Projects
None yet
3 participants
Contributor

whitequark commented Mar 26, 2012

No description provided.

Owner

activestylus commented Mar 27, 2012

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

Contributor

whitequark commented Mar 27, 2012

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.

Owner

activestylus commented Mar 27, 2012

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

Contributor

whitequark commented Mar 27, 2012

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 Sep 6, 2013

Sounds great, is something missing for the merge?

Owner

activestylus commented Sep 8, 2013

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

Thanks!

activestylus reopened this Sep 8, 2013

@activestylus activestylus added a commit that referenced this pull request Sep 8, 2013

@activestylus activestylus Merge pull request #3 from whitequark/master
Added ActiveRecord and improved styling hooks
5c1c269

@activestylus activestylus merged commit 5c1c269 into activestylus:master Sep 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment