Fixes #6825, adds tests covering cases and error possibilities #6873

Merged
merged 1 commit into from Jun 27, 2012

Conversation

Projects
None yet
3 participants
@mauricio
Contributor

mauricio commented Jun 27, 2012

The current implementation on Rails is not capable of handling an object with time only, it only allows you to create an object with both time and date, which isn't really nice as you can create a time only column in almost all database systems.

This fix adds support for time only object creation (if the column is created as :time) by using Ruby's Time object. The date fields will default to "January 1, 1970" since Ruby doesn't have an object for time only. Implementation also includes specs showing what's the correct behavior and also a failure if you try to create a time only object of a column of type datetime.

This pull request fixes issue #6825.

@mauricio

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor
@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jun 27, 2012

Member

It looks ok to me, lets just wait some more feedback before merging, thanks!

It looks ok to me, lets just wait some more feedback before merging, thanks!

@rafaelfranca

View changes

activerecord/lib/active_record/attribute_assignment.rb
+ end
+ return nil if (1..3).any? { |position| values_hash_from_param[position].blank? }
+ end
+
max_position = extract_max_param_for_multiparameter_attributes(values_hash_from_param, 6)
# If Date bits were provided but blank, then return nil

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

I think this comment should be moved with 196

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

I think this comment should be moved with 196

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

Not following. What comment?

@mauricio

mauricio Jun 27, 2012

Contributor

Not following. What comment?

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member
# If Date bits were provided but blank, then return nil

Should be

if missing_parameter = [1,2,3].detect{ |position| !values_hash_from_param.has_key?(position) }
  raise ArgumentError.new("Missing Parameter - #{name}(#{missing_parameter}i)")
end

# If Date bits were provided but blank, then return nil
return nil if (1..3).any? { |position| values_hash_from_param[position].blank? }
@rafaelfranca

rafaelfranca Jun 27, 2012

Member
# If Date bits were provided but blank, then return nil

Should be

if missing_parameter = [1,2,3].detect{ |position| !values_hash_from_param.has_key?(position) }
  raise ArgumentError.new("Missing Parameter - #{name}(#{missing_parameter}i)")
end

# If Date bits were provided but blank, then return nil
return nil if (1..3).any? { |position| values_hash_from_param[position].blank? }

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jun 27, 2012

Member

Ah missed that: # If Date bits were provided but blank, then return nil

@carlosantoniodasilva

carlosantoniodasilva Jun 27, 2012

Member

Ah missed that: # If Date bits were provided but blank, then return nil

@mauricio

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

I have force-pushed the same commit again and it's showing two here, not nice.

Contributor

mauricio commented Jun 27, 2012

I have force-pushed the same commit again and it's showing two here, not nice.

@rafaelfranca

View changes

activerecord/lib/active_record/attribute_assignment.rb
+ if missing_parameter = [1,2,3].detect{ |position| !values_hash_from_param.has_key?(position) }
+ raise ArgumentError.new("Missing Parameter - #{name}(#{missing_parameter}i)")
+ end
+ # If Date bits were provided but blank, then return nil

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Put a blank line before this comment. I think it make easier to read.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Put a blank line before this comment. I think it make easier to read.

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

Cool, anything else?

@mauricio

mauricio Jun 27, 2012

Contributor

Cool, anything else?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Yes, it is showing two commits.

I saw that you refactored activerecord/lib/active_record/errors.rb. I think is better to make it in a separate commit, in this pull request.

Member

rafaelfranca commented Jun 27, 2012

Yes, it is showing two commits.

I saw that you refactored activerecord/lib/active_record/errors.rb. I think is better to make it in a separate commit, in this pull request.

@mauricio

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

Separate commits? Shouldn't it all be squashed in a single commit?

The change in activerecord/lib/active_record/errors.rb is necessary to get the exception message to be correctly initialized, it wasn't loading the correct error before.

Contributor

mauricio commented Jun 27, 2012

Separate commits? Shouldn't it all be squashed in a single commit?

The change in activerecord/lib/active_record/errors.rb is necessary to get the exception message to be correctly initialized, it wasn't loading the correct error before.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

We only ask to squash if the commit does not make sense alone.

I thought that changes at activerecord/lib/active_record/errors.rb was only refactoring, but since it is required to initialize the message so it is fine to be done in the same commit.

So, put the blank line before the comment and squash all the commits that I'll merge.

Member

rafaelfranca commented Jun 27, 2012

We only ask to squash if the commit does not make sense alone.

I thought that changes at activerecord/lib/active_record/errors.rb was only refactoring, but since it is required to initialize the message so it is fine to be done in the same commit.

So, put the blank line before the comment and squash all the commits that I'll merge.

@mauricio

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

Cool, added the line and it's a single commit now.

Contributor

mauricio commented Jun 27, 2012

Cool, added the line and it's a single commit now.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

@mauricio now we have 3 commits. Very strange.

Member

rafaelfranca commented Jun 27, 2012

@mauricio now we have 3 commits. Very strange.

Fixes #6825, adds tests covering cases and error possibilities, also…
… changes SQLite3 driver to correctly generate a time column instead of datetime
@mauricio

This comment has been minimized.

Show comment Hide comment
@mauricio

mauricio Jun 27, 2012

Contributor

Whoops, fixed.

Contributor

mauricio commented Jun 27, 2012

Whoops, fixed.

rafaelfranca added a commit that referenced this pull request Jun 27, 2012

Merge pull request #6873 from mauricio/fix-for-6825
Fixes #6825, adds tests covering cases and error possibilities

@rafaelfranca rafaelfranca merged commit 2076efe into rails:master Jun 27, 2012

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