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

Take into consideration active record time zone aware flag #202

Closed
wants to merge 5 commits into from

Conversation

camilova
Copy link

When active record has the flag config.active_record.time_zone_aware_attributes setted to false time zone aware should not used, and same case when attribute class is not included on classes with time zone aware defined by AR too.

@adzap
Copy link
Owner

adzap commented Jan 25, 2022

Thanks for this. This PR confused me at first but it demonstrates a few issues that exist in the gem which need fixing.

Which are:

  1. That AR is expected in the validator class despite this gem offering ActiveModel support rather than AR specifically
  2. That the gem implicitly expects a timezone aware attributes, which is not intended
  3. The gem doesn't check time_zone_aware_types at all which is problematic if you want to control that

But your implementation is not quite what I'd like, so I'm going to review whether some of this AR specific code can be moved to an ORM extension.

@camilova
Copy link
Author

Thank you @adzap, I will close this in favor of an ORM extension that I think is a better approach than this.

@camilova camilova closed this Jan 25, 2022
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.

2 participants