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

Convert stale? and fresh_when to use keyword arguments. #18872

Merged
merged 1 commit into from Feb 10, 2015

Conversation

Projects
None yet
6 participants
Show outdated Hide outdated actionpack/lib/action_controller/metal/conditional_get.rb
record = record_or_options
options = { etag: record, last_modified: record.try(:updated_at) }.merge!(additional_options)
def fresh_when(record = nil, etag: record, last_modified: nil, public: false, template: nil)
last_modified ||= record.updated_at if record

This comment has been minimized.

@arthurnn

arthurnn Feb 10, 2015

Member

can we safely remote the try call that was in here before?

@arthurnn

arthurnn Feb 10, 2015

Member

can we safely remote the try call that was in here before?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

This is not what the if record does?

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

This is not what the if record does?

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

On second thought I'm not sure.
I don't know what methods it takes for a record to be compliant for this API, so supposedly you could pass an Active Model model which wouldn't respond to #updated_at.

@kaspth

kaspth Feb 10, 2015

Member

On second thought I'm not sure.
I don't know what methods it takes for a record to be compliant for this API, so supposedly you could pass an Active Model model which wouldn't respond to #updated_at.

This comment has been minimized.

@claudiob

claudiob Feb 10, 2015

Member

@kaspth 🎉 I was going to write this PR myself but you did almost everything! 👍

Regarding the updated_at, you cannot safely remove the try if you want this change to be backward-compatible

@claudiob

claudiob Feb 10, 2015

Member

@kaspth 🎉 I was going to write this PR myself but you did almost everything! 👍

Regarding the updated_at, you cannot safely remove the try if you want this change to be backward-compatible

This comment has been minimized.

@claudiob

claudiob Feb 10, 2015

Member

However, there are currently no tests that invoke fresh_when or stale? with an object that does not define updated_at, and even the documentation says:

You can also just pass a record where last_modified will be set by calling updated_at and the etag by passing the object itself

So removing the try might be acceptable according to both tests and docs.
If you decide to do so, I suggest you remove it in a separate commit, because everything else
in your commit is 100% backward-compatible

@claudiob

claudiob Feb 10, 2015

Member

However, there are currently no tests that invoke fresh_when or stale? with an object that does not define updated_at, and even the documentation says:

You can also just pass a record where last_modified will be set by calling updated_at and the etag by passing the object itself

So removing the try might be acceptable according to both tests and docs.
If you decide to do so, I suggest you remove it in a separate commit, because everything else
in your commit is 100% backward-compatible

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

@claudiob Thanks for the investigation ❤️

I'll leave the try in there, then we can always discuss whether it's still needed later.

@kaspth

kaspth Feb 10, 2015

Member

@claudiob Thanks for the investigation ❤️

I'll leave the try in there, then we can always discuss whether it's still needed later.

Show outdated Hide outdated actionpack/lib/action_controller/metal/conditional_get.rb
@@ -157,8 +155,8 @@ def fresh_when(record_or_options, additional_options = {})
# super if stale? @article, template: 'widgets/show'
# end
#
def stale?(record_or_options, additional_options = {})
fresh_when(record_or_options, additional_options)
def stale?(record = nil, **options)

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

For documentation purpose make the options explicit.

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

For documentation purpose make the options explicit.

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

You're right that's better.

@kaspth

kaspth Feb 10, 2015

Member

You're right that's better.

response.last_modified = options[:last_modified] if options[:last_modified]
response.cache_control[:public] = true if options[:public]
response.last_modified = last_modified if last_modified
response.cache_control[:public] = true if public

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

Is not public a reserved word?

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

Is not public a reserved word?

This comment has been minimized.

@arthurnn

arthurnn Feb 10, 2015

Member

public is reserved?

@arthurnn

arthurnn Feb 10, 2015

Member

public is reserved?

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

It looks weird, but it worked fine when I ran the tests locally and soon on Travis.

@kaspth

kaspth Feb 10, 2015

Member

It looks weird, but it worked fine when I ran the tests locally and soon on Travis.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

So it is not

@rafaelfranca

rafaelfranca Feb 10, 2015

Member

So it is not

This comment has been minimized.

@vipulnsward

vipulnsward Feb 10, 2015

Member

public is similar to private. Its a bad idea to do this, since this would override definition of public

@vipulnsward

vipulnsward Feb 10, 2015

Member

public is similar to private. Its a bad idea to do this, since this would override definition of public

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

It's okay when it's only within the method. Besides we can cross that bridge when stale? or fresh_when needs to define something publicly within itself (which I can't imagine).

@kaspth

kaspth Feb 10, 2015

Member

It's okay when it's only within the method. Besides we can cross that bridge when stale? or fresh_when needs to define something publicly within itself (which I can't imagine).

This comment has been minimized.

@arthurnn

arthurnn Feb 10, 2015

Member

I am fine with the public .

@arthurnn

arthurnn Feb 10, 2015

Member

I am fine with the public .

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 10, 2015

Member

There's a Railties failure in a test that tries running bundle install on a newly generated app and master is green.

Could this be an intermittent failure?

Member

kaspth commented Feb 10, 2015

There's a Railties failure in a test that tries running bundle install on a newly generated app and master is green.

Could this be an intermittent failure?

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Feb 10, 2015

Member

@kaspth Try to modify your commit and push your branch again, might be solved by now.

Member

claudiob commented Feb 10, 2015

@kaspth Try to modify your commit and push your branch again, might be solved by now.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Feb 10, 2015

Member

Railties is not related... I am fixing that on master!

Member

arthurnn commented Feb 10, 2015

Railties is not related... I am fixing that on master!

@@ -157,8 +155,8 @@ def fresh_when(record_or_options, additional_options = {})
# super if stale? @article, template: 'widgets/show'
# end
#
def stale?(record_or_options, additional_options = {})
fresh_when(record_or_options, additional_options)
def stale?(record = nil, etag: nil, last_modified: nil, public: nil, template: nil)

This comment has been minimized.

@arthurnn

arthurnn Feb 10, 2015

Member

should etag: record ?

@arthurnn

arthurnn Feb 10, 2015

Member

should etag: record ?

This comment has been minimized.

@kaspth

kaspth Feb 10, 2015

Member

fresh_when defaults etag to record, so it shouldn't be a problem.

@kaspth

kaspth Feb 10, 2015

Member

fresh_when defaults etag to record, so it shouldn't be a problem.

This comment has been minimized.

@claudiob

claudiob Feb 11, 2015

Member

@kaspth @arthurnn I think it's a problem and I opened #18884 to solve it.

In short, if someone calls stale? without the etag parameter, then stale? will call fresh_when with etag: nil.

This will prevent the default value of etag: record set in fresh_when from being used, since nil will be used instead.

@claudiob

claudiob Feb 11, 2015

Member

@kaspth @arthurnn I think it's a problem and I opened #18884 to solve it.

In short, if someone calls stale? without the etag parameter, then stale? will call fresh_when with etag: nil.

This will prevent the default value of etag: record set in fresh_when from being used, since nil will be used instead.

This comment has been minimized.

@arthurnn

arthurnn Feb 11, 2015

Member

thats right @claudiob .. thanks brow! ❤️

@arthurnn

arthurnn Feb 11, 2015

Member

thats right @claudiob .. thanks brow! ❤️

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 10, 2015

Member

Build passed! That merge button is mighty tempting, but I'm not allowed 😄

Member

kaspth commented Feb 10, 2015

Build passed! That merge button is mighty tempting, but I'm not allowed 😄

arthurnn added a commit that referenced this pull request Feb 10, 2015

Merge pull request #18872 from kaspth/kw-fresh_when-stale
Convert stale? and fresh_when to use keyword arguments.

@arthurnn arthurnn merged commit 162581a into rails:master Feb 10, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Feb 10, 2015

Member

thanks ❤️ !
code looks much better!

Member

arthurnn commented Feb 10, 2015

thanks ❤️ !
code looks much better!

@kaspth kaspth deleted the kaspth:kw-fresh_when-stale branch Feb 10, 2015

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 10, 2015

Member

❤️ all around!

Member

kaspth commented Feb 10, 2015

❤️ all around!

claudiob added a commit that referenced this pull request Feb 11, 2015

Fix wrong kwarg "record" from #18872
PR #18772 changed the parameters of `stale?` to use `kwargs`.
[As for this comment](https://github.com/rails/rails/pull/18872/files#r24456288)
the default value for the `etag` parameter should be `record`, not `nil`.

This commit fixes the code and introduces a test that:

- passed before #18872
- fails on the current master (after #18772)
- passes again after setting the default value of `etag` to `record`.

rafaelfranca added a commit that referenced this pull request Feb 11, 2015

@@ -77,18 +77,16 @@ def etag(&etagger)
#
# before_action { fresh_when @article, template: 'widgets/show' }
#
def fresh_when(record_or_options, additional_options = {})
if record_or_options.is_a? Hash

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 11, 2015

Member

The old implementation suggests I could pass in directly a hash instead of a record, even thought there are no docs about it, there might be people using it that way.

Will this still work?

@carlosantoniodasilva

carlosantoniodasilva Feb 11, 2015

Member

The old implementation suggests I could pass in directly a hash instead of a record, even thought there are no docs about it, there might be people using it that way.

Will this still work?

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

If you pass a hash as the first argument Ruby will use the hash as the keyword arguments because record defaults to nil.
See here:

irb(main):001:0> def old_fresh_when(record_or_options, additional_options = {})
irb(main):002:1>   puts record_or_options
irb(main):003:1>   puts additional_options
irb(main):004:1> end
=> :old_fresh_when
irb(main):005:0> old_fresh_when({})
{}
{}
=> nil
irb(main):006:0> def fresh_when(record = nil, **options)
irb(main):007:1>   puts record
irb(main):008:1>   puts options
irb(main):009:1> end
=> :fresh_when
irb(main):010:0> fresh_when(etag: 'hello')
# this line is empty because record is nil.
{:etag=>"hello"}
@kaspth

kaspth Feb 11, 2015

Member

If you pass a hash as the first argument Ruby will use the hash as the keyword arguments because record defaults to nil.
See here:

irb(main):001:0> def old_fresh_when(record_or_options, additional_options = {})
irb(main):002:1>   puts record_or_options
irb(main):003:1>   puts additional_options
irb(main):004:1> end
=> :old_fresh_when
irb(main):005:0> old_fresh_when({})
{}
{}
=> nil
irb(main):006:0> def fresh_when(record = nil, **options)
irb(main):007:1>   puts record
irb(main):008:1>   puts options
irb(main):009:1> end
=> :fresh_when
irb(main):010:0> fresh_when(etag: 'hello')
# this line is empty because record is nil.
{:etag=>"hello"}

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 11, 2015

Member

Gotcha, for some reason in my mind that'd only work with ** not individual declarations, 👍

@carlosantoniodasilva

carlosantoniodasilva Feb 11, 2015

Member

Gotcha, for some reason in my mind that'd only work with ** not individual declarations, 👍

This comment has been minimized.

@kaspth

kaspth Feb 11, 2015

Member

Yeah, keyword arguments, for all their niceties, have some logical warts 😁

@kaspth

kaspth Feb 11, 2015

Member

Yeah, keyword arguments, for all their niceties, have some logical warts 😁

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