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.

Sign up for GitHub

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

Jump to bottom

Allow :precision option for time type columns #18886

Merged
merged 1 commit into from Feb 12, 2015
Merged

Allow :precision option for time type columns #18886

merged 1 commit into from Feb 12, 2015

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Feb 11, 2015

No description provided.

@kamipo kamipo force-pushed the allow_precision_option_for_time_column branch 2 times, most recently from ec84082 to 8c96db9 Compare February 11, 2015 14:26
sgrif
sgrif reviewed Feb 11, 2015
View reviewed changes
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
when nil, 0..6; super
else
native_type = native_database_types[type.to_sym][:name]
raise(ActiveRecordError, "No #{native_type} type has precision of #{precision}. The allowed range of precision is from 0 to 6")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move this behavior to the abstract adapter, since it looks to be the same?

@sgrif
Copy link
Contributor

sgrif commented Feb 11, 2015

I just pushed up some pretty major code changes, this PR might be affected pretty heavily by it.

@kamipo kamipo force-pushed the allow_precision_option_for_time_column branch 2 times, most recently from 19ff882 to 2ddeee1 Compare February 11, 2015 22:10
@kamipo kamipo force-pushed the allow_precision_option_for_time_column branch from 2ddeee1 to 1502cae Compare February 11, 2015 22:24
@kamipo
Copy link
Member Author

kamipo commented Feb 11, 2015

Thank you for your review.
It is fixed and rebased on master.

rafaelfranca added a commit that referenced this pull request Feb 12, 2015
@rafaelfranca
Merge pull request #18886 from kamipo/allow_precision_option_for_time…
68ce508 
…_column

Allow `:precision` option for time type columns
@rafaelfranca rafaelfranca merged commit 68ce508 into rails:master Feb 12, 2015
@kamipo kamipo deleted the allow_precision_option_for_time_column branch February 12, 2015 18:15
@sgrif
Copy link
Contributor

sgrif commented Feb 17, 2015

This breaks the test suite when using the mysql adapter on MySQL 5.6 (Travis uses 5.5). I've reverted if you'd like to take another swing at this.

@kamipo
Copy link
Member Author

kamipo commented Feb 18, 2015

The cause by which the test suite for the mysql adapter broke in 1502cae
(reverted 89ba5bb) is because the precision was not extracted.

This will fix by #18979.

@rafaelfranca
Copy link
Member

@kamipo #18979 was merged. Will you submit this change again?

@kamipo
Copy link
Member Author

kamipo commented Feb 19, 2015

@rafaelfranca This change has been cherry-picked for was needed for #18914. Is it better to be split as another PR?

@rafaelfranca
Copy link
Member

Right. There is no need.

On Thu Feb 19 2015 at 10:17:52 AM Ryuta Kamizono notifications@github.com
wrote:

@rafaelfranca https://github.com/rafaelfranca This change has been
cherry-picked for was needed for #18914
#18914. Is it better to be split as
another PR?

—
Reply to this email directly or view it on GitHub
#18886 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kamipo @sgrif @rafaelfranca