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

Added content_for? check, no tests though #662

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mlightner
Contributor

mlightner commented Sep 8, 2011

No description provided.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Sep 8, 2011

Interesting, why rescue true?

DAddYE commented on 5d5e89f Sep 8, 2011

Interesting, why rescue true?

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Sep 8, 2011

Much better some like:

!(blocks.nil? || blocks.empty?)

DAddYE replied Sep 8, 2011

Much better some like:

!(blocks.nil? || blocks.empty?)

This comment has been minimized.

Show comment
Hide comment
@mlightner

mlightner Sep 8, 2011

Force of habit. Just in case blocks is empty. Can you call .empty? on a nil object? I'm just paranoid of getting caught with a nil object unexpectedly. Irrationally. :)

mlightner replied Sep 8, 2011

Force of habit. Just in case blocks is empty. Can you call .empty? on a nil object? I'm just paranoid of getting caught with a nil object unexpectedly. Irrationally. :)

This comment has been minimized.

Show comment
Hide comment
@mlightner

mlightner Sep 8, 2011

New commit with your code pushed.

mlightner replied Sep 8, 2011

New commit with your code pushed.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Sep 8, 2011

Member

Sounds much better now! Great feature, try to add almost one spec and we will merge soon.

Thanks man!

Member

DAddYE commented Sep 8, 2011

Sounds much better now! Great feature, try to add almost one spec and we will merge soon.

Thanks man!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 8, 2011

Member

Yea this is a good idea, but we need a spec before merge.

Member

nesquena commented Sep 8, 2011

Yea this is a good idea, but we need a spec before merge.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 8, 2011

Member

Also couldn't we just use blocks.present?

Member

nesquena commented Sep 8, 2011

Also couldn't we just use blocks.present?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Sep 10, 2011

Member

Look at this code! It's beautiful!

def content_for?(key)
  content_blocks[key.to_sym].try :any?
end
Member

ujifgc commented Sep 10, 2011

Look at this code! It's beautiful!

def content_for?(key)
  content_blocks[key.to_sym].try :any?
end
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 10, 2011

Member

@ujifgc That works too, I am just curious why one would use that over content_blocks[key.to_sym].present? both require activesupport extensions.

Member

nesquena commented Sep 10, 2011

@ujifgc That works too, I am just curious why one would use that over content_blocks[key.to_sym].present? both require activesupport extensions.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 10, 2011

Member

But .try is not part of ruby... http://rubydoc.info/docs/rails/Object:try or at least not every version

Member

nesquena commented Sep 10, 2011

But .try is not part of ruby... http://rubydoc.info/docs/rails/Object:try or at least not every version

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 10, 2011

Member

Merged with tests for all three major template systems in the above commit. Thanks!

Member

nesquena commented Sep 10, 2011

Merged with tests for all three major template systems in the above commit. Thanks!

@nesquena nesquena closed this Sep 10, 2011

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Sep 10, 2011

Member

@nesquena oh, good to know, thanks.

Member

ujifgc commented Sep 10, 2011

@nesquena oh, good to know, thanks.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 10, 2011

Member

Sure, thanks for your input and thanks to @mlightner for the patch.

Member

nesquena commented Sep 10, 2011

Sure, thanks for your input and thanks to @mlightner for the patch.

@mlightner

This comment has been minimized.

Show comment
Hide comment
@mlightner

mlightner Sep 10, 2011

Contributor

Thanks to all!

Contributor

mlightner commented Sep 10, 2011

Thanks to all!

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