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

created :time option for touch #18956

Merged
merged 1 commit into from Feb 18, 2015
Merged

created :time option for touch #18956

merged 1 commit into from Feb 18, 2015

Conversation

@hjoo
Copy link
Contributor

hjoo commented Feb 16, 2015

Created a ':time' option for '#touch' to address issue #18905. Not sure if there is a better method for implementing optional arguments.

@recursive-madman
Copy link
Contributor

recursive-madman commented Feb 16, 2015

Actually there is: keyword arguments:

def touch(*names, time: nil)
  if time
    # ...
  end
end

Alternatively, 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
@carlosantoniodasilva
carlosantoniodasilva reviewed Feb 16, 2015
View changes
activerecord/lib/active_record/persistence.rb Outdated
@@ -460,14 +460,19 @@ def reload(options = nil)
# ball = Ball.new
# ball.touch(:updated_at) # => raises ActiveRecordError
#
def touch(*names)
def touch(*args)

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Feb 16, 2015

Member

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?

@eileencodes
eileencodes reviewed Feb 16, 2015
View changes
activerecord/lib/active_record/persistence.rb Outdated
current_time = time_opt[0][:time]
else
current_time = current_time_from_proper_timezone
end

This comment has been minimized.

Copy link
@eileencodes

eileencodes Feb 16, 2015

Member

It looks like your tab indentation is set to 4 spaces and it should be 2 spaces. Let me know what your text editor is if you can't figure out how to change your indentation.

@eileencodes
eileencodes reviewed Feb 16, 2015
View changes
activerecord/lib/active_record/persistence.rb Outdated
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)}

This comment has been minimized.

Copy link
@eileencodes

eileencodes Feb 16, 2015

Member

Ruby style here should have spaces after the curly braces

time_opt, names = args.partition{ |arg| arg.kind_of?(Hash) && arg.has_key?(:time) }

@hjoo
Copy link
Contributor Author

hjoo commented Feb 16, 2015

Thanks for the feedback! I added time as a keyword argument and made sure the indentation was 2 spaces.

@eileencodes
Copy link
Member

eileencodes commented Feb 16, 2015

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 #touch. This also needs a test using the new argument option. Additionally the documentation above the touch method should show how to use the time argument.

@hjoo hjoo force-pushed the hjoo:time_option branch Feb 16, 2015
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

This comment has been minimized.

Copy link
@yawboakye

yawboakye Feb 17, 2015

Contributor

why not current_time = time?

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 17, 2015

Member

It's easier to use the time argument and reassigning it to current_time doesn't improve clarity 😉

@eileencodes
eileencodes reviewed Feb 17, 2015
View changes
activerecord/CHANGELOG.md Outdated
Fixes #18905.

*Hyonjee Joo*

This comment has been minimized.

Copy link
@eileencodes

eileencodes Feb 17, 2015

Member

@hjoo can you move this entry to the top of the file? Thanks!

@carlosantoniodasilva
carlosantoniodasilva reviewed Feb 17, 2015
View changes
activerecord/CHANGELOG.md Outdated
@@ -543,4 +543,9 @@

*Yves Senn*

* ':time' option added for '#touch'

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Feb 17, 2015

Member

You can wrap these in backticks instead of single quotes, so that it highlights properly as markdown. You can see the formatted file in github.

@eileencodes
Copy link
Member

eileencodes commented Feb 17, 2015

@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:

Add `time` option to `#touch`

Fixes #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. 

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:
git commit --amend

After editing and saving the new commit message force push again to your branch.
git push -f origin your-branch

@hjoo hjoo force-pushed the hjoo:time_option branch 3 times, most recently Feb 17, 2015
Fixes #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.
@hjoo hjoo force-pushed the hjoo:time_option branch to 219d71f Feb 18, 2015
eileencodes added a commit that referenced this pull request Feb 18, 2015
Add `time` option to `#touch
@eileencodes eileencodes merged commit 83be869 into rails:master Feb 18, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeremy
Copy link
Member

jeremy commented Feb 18, 2015

🎉 Congrats @hjoo !

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.