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

Allow fragment cache to accept :if and :unless options #8371

Merged
merged 1 commit into from Dec 5, 2012

Conversation

Projects
None yet
4 participants
@freegenie
Contributor

freegenie commented Nov 29, 2012

No description provided.

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Nov 29, 2012

Contributor

This issue was discussed here #5396

Contributor

freegenie commented Nov 29, 2012

This issue was discussed here #5396

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2012

Member

I think the implementation on #5396 is better.

Member

rafaelfranca commented Nov 29, 2012

I think the implementation on #5396 is better.

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Nov 29, 2012

Contributor

@rafaelfranca please tell me more of your point of view so that I can learn what's wrong

Contributor

freegenie commented Nov 29, 2012

@rafaelfranca please tell me more of your point of view so that I can learn what's wrong

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2012

Member

I think this is simpler.

def options_pass_conditions?(options) #:nodoc:
  !(options && (!options.fetch(:if, true) || options.fetch(:unless, false)))
end

Or this doesn't work with your implementation?

Member

rafaelfranca commented Nov 29, 2012

I think this is simpler.

def options_pass_conditions?(options) #:nodoc:
  !(options && (!options.fetch(:if, true) || options.fetch(:unless, false)))
end

Or this doesn't work with your implementation?

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Nov 29, 2012

Contributor

well, you are right, it's much simpler, I fixed it and pushed again.

Contributor

freegenie commented Nov 29, 2012

well, you are right, it's much simpler, I fixed it and pushed again.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2012

Member

Why not use the same logic?

Member

rafaelfranca commented Nov 29, 2012

Why not use the same logic?

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Nov 29, 2012

Contributor

yes you are right, I used fetch because it's much dryier and readable to me, pushed again. I'm using --force to keep it in one commit.

Contributor

freegenie commented Nov 29, 2012

yes you are right, I used fetch because it's much dryier and readable to me, pushed again. I'm using --force to keep it in one commit.

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Dec 5, 2012

Contributor

@pixeltrix any opinion on this one too?

Contributor

freegenie commented Dec 5, 2012

@pixeltrix any opinion on this one too?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 5, 2012

Member

I'm definitely 👍 on it - I've done something similar in my apps where for example I've cached the rendering of an empty shopping basket but not anything else as the cache hit rate would be low so anything which is user customisable but has a common default is definitely a good match for this.

I'll have a closer look later but it'll need a CHANGELOG entry and the commits squashing on first glance.

Member

pixeltrix commented Dec 5, 2012

I'm definitely 👍 on it - I've done something similar in my apps where for example I've cached the rendering of an empty shopping basket but not anything else as the cache hit rate would be low so anything which is user customisable but has a common default is definitely a good match for this.

I'll have a closer look later but it'll need a CHANGELOG entry and the commits squashing on first glance.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 5, 2012

Member

I didn't undertand why did you open a new pull request with the same code of #5396 without ask to the original author if it is still interested in the code.

Besides this, (both) pull requests look good and only need the CHANGELOG entry and the commits squashed.

Member

rafaelfranca commented Dec 5, 2012

I didn't undertand why did you open a new pull request with the same code of #5396 without ask to the original author if it is still interested in the code.

Besides this, (both) pull requests look good and only need the CHANGELOG entry and the commits squashed.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 5, 2012

Member

Does the test still fail in the way mentioned in #5396?

Member

pixeltrix commented Dec 5, 2012

Does the test still fail in the way mentioned in #5396?

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Dec 5, 2012

Contributor

@rafaelfranca sorry about that, I should have asked, you are right. I didn't mean to cheat anybody, I just thought it was easier for me to start from a white page on a branch of my own. And maybe I considered two months inactivity as enough time for anybody to be no longer interested in the code. Please go ahead with whatever you feel to be more correct to resolve this pull request.

Contributor

freegenie commented Dec 5, 2012

@rafaelfranca sorry about that, I should have asked, you are right. I didn't mean to cheat anybody, I just thought it was easier for me to start from a white page on a branch of my own. And maybe I considered two months inactivity as enough time for anybody to be no longer interested in the code. Please go ahead with whatever you feel to be more correct to resolve this pull request.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 5, 2012

Member

@freegenie no problem. You did an awesome work and deserves recognition. Also, your tests are passing.

To be fair change you commit message and put this in the last line:

[Stephen Ausman + Fabrizio Regini]

And we can merge this one.

Thank you so much for the pull request.

Member

rafaelfranca commented Dec 5, 2012

@freegenie no problem. You did an awesome work and deserves recognition. Also, your tests are passing.

To be fair change you commit message and put this in the last line:

[Stephen Ausman + Fabrizio Regini]

And we can merge this one.

Thank you so much for the pull request.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 5, 2012

Member

Ah, if you could put the Stephen Ausman name in the CHANGELOG too

Member

rafaelfranca commented Dec 5, 2012

Ah, if you could put the Stephen Ausman name in the CHANGELOG too

@freegenie

This comment has been minimized.

Show comment
Hide comment
@freegenie

freegenie Dec 5, 2012

Contributor

@rafaelfranca done, thanks for teaching!

Contributor

freegenie commented Dec 5, 2012

@rafaelfranca done, thanks for teaching!

rafaelfranca added a commit that referenced this pull request Dec 5, 2012

Merge pull request #8371 from freegenie/5396-conditional-fragment-cac…
…hing

Allow fragment cache to accept :if and :unless options.

Closes #5396

@rafaelfranca rafaelfranca merged commit 6ed4ad1 into rails:master Dec 5, 2012

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 5, 2012

Member

Thank you so much for the pull request. ❤️

Member

rafaelfranca commented Dec 5, 2012

Thank you so much for the pull request. ❤️

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 5, 2012

Member

@rafaelfranca hey, you stole my merge commit :trollface:

Member

pixeltrix commented Dec 5, 2012

@rafaelfranca hey, you stole my merge commit :trollface:

@@ -136,6 +143,11 @@ def cache_fragment_name(name = {}, options = nil)
end
private
def conditions_match?(options)
!(options && (!options.fetch(:if, true) || options.fetch(:unless, false)))

This comment has been minimized.

@bogdan

bogdan Dec 5, 2012

Contributor

I thought there is :force => true option supported by ActiveSupport::CacheStore to do this. I believe it is supported at this layer as well.

@bogdan

bogdan Dec 5, 2012

Contributor

I thought there is :force => true option supported by ActiveSupport::CacheStore to do this. I believe it is supported at this layer as well.

This comment has been minimized.

@pixeltrix

pixeltrix Dec 5, 2012

Member

@bogdan that's slightly different - it forces a cache miss so it will still write the newly rendered fragment. This will not cache the fragment at all.

@pixeltrix

pixeltrix Dec 5, 2012

Member

@bogdan that's slightly different - it forces a cache miss so it will still write the newly rendered fragment. This will not cache the fragment at all.

This comment has been minimized.

@bogdan

bogdan Dec 5, 2012

Contributor

@pixeltrix clear enough

Than maybe we should support :if and :unless in CacheStore. wdyt?

@bogdan

bogdan Dec 5, 2012

Contributor

@pixeltrix clear enough

Than maybe we should support :if and :unless in CacheStore. wdyt?

This comment has been minimized.

@pixeltrix

pixeltrix Dec 5, 2012

Member

Possibly - can you think of a use case?

@pixeltrix

pixeltrix Dec 5, 2012

Member

Possibly - can you think of a use case?

This comment has been minimized.

@bogdan

bogdan Dec 5, 2012

Contributor

Sure.

Test environment can create object with same id many times during test suite

Now I do:

  Rails.cache.fetch(object.id, :force => Rails.env.test?) do

  end

But using :if will be better.

@bogdan

bogdan Dec 5, 2012

Contributor

Sure.

Test environment can create object with same id many times during test suite

Now I do:

  Rails.cache.fetch(object.id, :force => Rails.env.test?) do

  end

But using :if will be better.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 5, 2012

Member

@pixeltrix so sorry 🙇

Member

rafaelfranca commented Dec 5, 2012

@pixeltrix so sorry 🙇

acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 14, 2012

Removed :if and :unless from fragment cache option in favour of
cache_if(condition, option, &block) and cache_unless(condition, option, &block).

In the PR #8371 was introduced  conditional options :if and :unless in
the cache method.

    Example:

      <%= cache @model, if: some_condition(@model) do %>
        ...
      <%end%>

This is a good feature but *cache_if* and and *cache_unless*
are more concise and close to the standard of rails view helpers
(ex: link_to_if and link_to_unless).

    Example:

      <%= cache_if condition, @model do %>
      ...
      <%end%>

acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 14, 2012

Removed :if and :unless from fragment cache option in favour of
cache_if(condition, option, &block) and cache_unless(condition, option, &block).

In the PR #8371 was introduced  conditional options :if and :unless in
the cache method.

    Example:

      <%= cache @model, if: some_condition(@model) do %>
        ...
      <%end%>

This is a good feature but *cache_if* and and *cache_unless*
are more concise and close to the standard of rails view helpers
(ex: link_to_if and link_to_unless).

    Example:

      <%= cache_if condition, @model do %>
      ...
      <%end%>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment