created :time option for touch #18956
Conversation
|
Actually there is: keyword arguments: def touch(*names, time: nil)
if time
# ...
end
endAlternatively, you can use Array#extract_options!, which active support adds to the core Array class: def touch(*names)
options = names.extract_options!
if options[:time]
# ...
end
end |
| @@ -460,14 +460,19 @@ def reload(options = nil) | |||
| # ball = Ball.new | |||
| # ball.touch(:updated_at) # => raises ActiveRecordError | |||
| # | |||
| def touch(*names) | |||
| def touch(*args) | |||
There was a problem hiding this comment.
I think you should be able to use keyword arguments as @recursive-madman commented, something like this:
def touch(*names, time: current_time_from_proper_timezone)And then change the local var from current_time to this new time one, should allow you to remove the need for the unless condition. What do you think?
| raise ActiveRecordError, "cannot touch on a new record object" unless persisted? | ||
|
|
||
| attributes = timestamp_attributes_for_update_in_model | ||
| time_opt, names = args.partition{|arg| arg.kind_of?(Hash) && arg.has_key?(:time)} |
There was a problem hiding this comment.
Ruby style here should have spaces after the curly braces
time_opt, names = args.partition{ |arg| arg.kind_of?(Hash) && arg.has_key?(:time) }
|
Thanks for the feedback! I added time as a keyword argument and made sure the indentation was 2 spaces. |
|
This is looking great @hjoo - we need a couple more items to make this PR complete. Please add an entry to the Active Record CHANGELOG.md noting that you can now set a time in |
| raise ActiveRecordError, "cannot touch on a new record object" unless persisted? | ||
|
|
||
| attributes = timestamp_attributes_for_update_in_model | ||
| attributes.concat(names) | ||
|
|
||
| unless attributes.empty? | ||
| current_time = current_time_from_proper_timezone |
There was a problem hiding this comment.
why not current_time = time?
There was a problem hiding this comment.
It's easier to use the time argument and reassigning it to current_time doesn't improve clarity 😉
|
@hjoo I know we didn't go over this at the Open Academy hackathon - Commit messages should be 50 characters and additional description should be after that. So a better commit message would be: The idea being that GitHub could go away tomorrow but your commit messages are forever. Updating the changelog doesn't need to be noted, btw. Also, now someone knows what this change is doing without going to github. 😄 To update the commit message you can do: After editing and saving the new commit message force push again to your branch. |
2a01aee to
c0809f2
Compare
Fixes rails#18905. `#touch` now takes time as an option. Setting the option saves the record with the updated_at/on attributes set to the current time or the time specified. Updated tests and documentation accordingly.
Add `time` option to `#touch
|
🎉 Congrats @hjoo ! |
Created a ':time' option for '#touch' to address issue #18905. Not sure if there is a better method for implementing optional arguments.