Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature for field method, passing a type option will accepts any active record sql types and type casts the value when it is set. #84

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zeisler
Copy link

@zeisler zeisler commented Feb 9, 2014

This feature is helpful because I use ActiveHash for mock objects in my tests and this would allow them to more resemble ActiveRecord's functionality.

 will accepts any active record sql types and type casts the value when it is
 set.
@zilkey
Copy link
Collaborator

zilkey commented Feb 9, 2014

Thanks! That's a great idea.

So far active_hash doesn't depend on ActiveRecord (you can optionally include modules that depend on ActiveRecord, but core ActiveHash::Base doesn't know about AR), and I'd like to keep it that way. Your patch would require us to create that dependency as it stands.

I would be OK relying on Virtus (https://github.com/solnic/virtus) which is a much smaller library. Are you familiar with Virtus? I've also been considering moving all the active_record-related methods to a different gem.

Any thoughts?

@zeisler
Copy link
Author

zeisler commented Feb 9, 2014

I have been looking into the Virtus code base and found
Virtus::Attribute.build(:type).coerce(:value). This can be easily swapped
in for the ActiveRecord Column. It will accept ruby class as types this
will be a little different from how AR does it, but should be close enough
and more useful in the general case when not using ActiveHash in the
context of the AR api. The only question I would have as far as
implementation is should the default value be coerced as well?

On Sat, Feb 8, 2014 at 8:29 PM, Jeff Dean [email protected] wrote:

Thanks! That's a great idea.

So far active_hash doesn't depend on ActiveRecord (you can optionally
include modules that depend on ActiveRecord, but core ActiveHash::Base
doesn't know about AR), and I'd like to keep it that way. Your patch would
require us to create that dependency as it stands.

I would be OK relying on Virtus (https://github.com/solnic/virtus) which
is a much smaller library. Are you familiar with Virtus? I've also been
considering moving all the active_record-related methods to a different gem.

Any thoughts?

Reply to this email directly or view it on GitHubhttps://github.com//pull/84#issuecomment-34565252
.

@zilkey
Copy link
Collaborator

zilkey commented Feb 9, 2014

Yes, I think defaults should also be coerced. If performance ends up being an issue, we should be able to solve it pretty easily.

Thanks!

On Feb 9, 2014, at 12:00 PM, Dustin Zeisler [email protected] wrote:

I have been looking into the Virtus code base and found
Virtus::Attribute.build(:type).coerce(:value). This can be easily swapped
in for the ActiveRecord Column. It will accept ruby class as types this
will be a little different from how AR does it, but should be close enough
and more useful in the general case when not using ActiveHash in the
context of the AR api. The only question I would have as far as
implementation is should the default value be coerced as well?

On Sat, Feb 8, 2014 at 8:29 PM, Jeff Dean [email protected] wrote:

Thanks! That's a great idea.

So far active_hash doesn't depend on ActiveRecord (you can optionally
include modules that depend on ActiveRecord, but core ActiveHash::Base
doesn't know about AR), and I'd like to keep it that way. Your patch would
require us to create that dependency as it stands.

I would be OK relying on Virtus (https://github.com/solnic/virtus) which
is a much smaller library. Are you familiar with Virtus? I've also been
considering moving all the active_record-related methods to a different gem.

Any thoughts?

Reply to this email directly or view it on GitHubhttps://github.com//pull/84#issuecomment-34565252
.


Reply to this email directly or view it on GitHub.

@zeisler
Copy link
Author

zeisler commented Feb 9, 2014

Looks like the virtus gem is failing under ruby 1.8.7

@zilkey
Copy link
Collaborator

zilkey commented Feb 9, 2014

Maybe it's time to release a version 2 of active hash that only supports ruby 1.9+.

Or maybe we could make the active record-esque type coercion a module, so it solves your case, but doesn't force us to add a hard dependency.

Any strong opinions?

On Feb 9, 2014, at 12:58 PM, Dustin Zeisler [email protected] wrote:

Looks like the virus gem is failing under ruby 1.8.7


Reply to this email directly or view it on GitHub.

@zeisler
Copy link
Author

zeisler commented Feb 9, 2014

I do like making it into a module you could give people a choice of which style they want to use either AR or Virtus, but that would add complexity. I am not sure if it's a problem now with AR but if it's kept up to date it will not support 1.8.7. Keeping AR at an older vesrion will not solve the case of matching the current AR api.

@kbrock
Copy link
Collaborator

kbrock commented Feb 10, 2014

I wonder if this would be failing under 1.8.7 if the axiom types gem didn't use the "new" lambda syntax (i.e.: ->). Not sure how much non 1.8.7 syntax is used.

Not sure if they would be receptive to using the old syntax. I know 1.9 has been end of life'd, but I'd imagine 1.8.7 may be around for a little while longer.

@zilkey
Copy link
Collaborator

zilkey commented Feb 10, 2014

I think it's probably reasonable to stop supporting 1.8.7 now with active hash. I think I'd like to close the open issues, release one more clean version, then roll this into a version 2, dropping support for 1.8.7.

On Feb 10, 2014, at 8:15 AM, Keenan Brock [email protected] wrote:

I wonder if this would be failing under 1.8.7 if the axiom types gem didn't use the "new" lambda syntax ->. Not sure how much non 1.8.7 syntax is used.

Not sure if they would be receptive to using the old syntax. I know 1.9 has been end of life'd, but I'd imagine 1.8.7 may be around for a little while longer.


Reply to this email directly or view it on GitHub.

@zilkey
Copy link
Collaborator

zilkey commented Feb 19, 2014

I just released 1.3.0, which included all the bug fixes I wanted to include before making any major changes. Unless there are any objections, I think I'll drop 1.8.7 support, and release a 2.0 with these changes.

@zilkey
Copy link
Collaborator

zilkey commented Feb 19, 2014

@zeisler - would you mind squashing those commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants