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

Accept a collection in fresh_when and stale? #18374

Merged
merged 1 commit into from
Feb 11, 2015

Conversation

claudiob
Copy link
Member

@claudiob claudiob commented Jan 7, 2015

The methods fresh_when and stale? from ActionController::ConditionalGet accept a single record as a short form for a hash (see docs). For instance:

  def show
    @article = Article.find(params[:id])
    fresh_when(@article)
  end

is just a short form for:

  def show
    @article = Article.find(params[:id])
    fresh_when(etag: @article, last_modified: @article.created_at)
  end

This commit extends fresh_when and stale? to also accept a collection of records, so that a short form similar to the one above can be used in an index action.
After this commit, the following code:

def index
  @article = Article.all
  fresh_when(etag: @articles, last_modified: @articles.maximum(:created_at))
end

can be simply written as:

def index
  @article = Article.all
  fresh_when(@articles)
end

options.assert_valid_keys(:etag, :last_modified, :public, :template)
elsif record_or_collection_or_options.respond_to? :maximum
records = record_or_collection_or_options
options = { etag: records, last_modified: records.try(:maximum, :updated_at) }.merge!(additional_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you already check respond_to :maximum, then do you still need records.try here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@egilburg I had doubts about this too.

The reason why the code is written like that is: what if collection has a :maximum with a different arity than one? For instance:

class Collection
  def maximum
    100
  end
end

In this case, I wouldn't want an ArgumentError raised… simply caching wouldn't work.

What do you think? Would you rewrite this type of check in a different way?

Copy link
Contributor

Choose a reason for hiding this comment

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

If a developer overrides a system method and changes the method signature, they are just asking for trouble :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides, try doesn't handle arity differences anyways:

'asdf'.try(:gsub)
ArgumentError: wrong number of arguments (0 for 1..2)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok! I'll remove the try

@claudiob claudiob force-pushed the add-collection-to-fresh-when branch 3 times, most recently from ab762c0 to c0ea4fb Compare January 12, 2015 06:26
@claudiob
Copy link
Member Author

claudiob commented Feb 9, 2015

Good morning @rafaelfranca ☀️
Any opinion on this?

I think that's something that would help me and other developers too, since combining fresh_when and .maximum is something that I've seen in several places such as:

@@ -157,8 +186,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_or_collection_or_options, additional_options = {})
Copy link
Member

Choose a reason for hiding this comment

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

record_or_collection_or_options this is getting out of hand =).
I would say this is a good opportunity to change the options passed to conditional_get to use kwargs! As far as I can tell we always validate by :etag, :last_modified, :public, :template , so those should be the args!

I also would say that refactor should happen first, and then we come back to evaluate this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@arthurnn #18872 is ready for review 😄

@arthurnn
Copy link
Member

@claudiob , wanna rebase this on latest master and try again?
thanks

The methods `fresh_when` and `stale?` from ActionController::ConditionalGet
accept a single record as a short form for a hash. For instance

```ruby
  def show
    @Article = Article.find(params[:id])
    fresh_when(@Article)
  end
```

is just a short form for:

```ruby
  def show
    @Article = Article.find(params[:id])
    fresh_when(etag: @Article, last_modified: @article.created_at)
  end
```

This commit extends `fresh_when` and `stale?` to also accept a collection
of records, so that a short form similar to the one above can be used in
an `index` action. After this commit, the following code:

```ruby
def index
  @Article = Article.all
  fresh_when(etag: @articles, last_modified: @articles.maximum(:created_at))
end
```

can be simply written as:

```ruby
def index
  @Article = Article.all
  fresh_when(@articles)
end
```
@claudiob
Copy link
Member Author

@arthurnn Done… now we can review 🎀

@kaspth
Copy link
Contributor

kaspth commented Feb 11, 2015

Looks good.

rafaelfranca added a commit that referenced this pull request Feb 11, 2015
Accept a collection in fresh_when and stale?
@rafaelfranca rafaelfranca merged commit e4b624a into rails:master Feb 11, 2015
@arthurnn
Copy link
Member

nice!! code looks much better now!

@claudiob claudiob deleted the add-collection-to-fresh-when branch February 11, 2015 21:04
# Before
def index
@article = Article.all
fresh_when(etag: @articles, last_modified: @articles.maximum(:created_at))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be maximum(:updated_at)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's fixed on master.

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

Successfully merging this pull request may close these issues.

6 participants