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

For MT: :entry_text_more attribute is nil or not before evaluate #45

Merged
merged 1 commit into from Aug 9, 2013

Conversation

Projects
None yet
5 participants
@shigeya
Contributor

shigeya commented Aug 8, 2013

This happen when I tried my MovableType blog import (still on going)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 8, 2013

Member

Oh great, thanks!

LGTM. @mattr-?

Member

parkr commented Aug 8, 2013

Oh great, thanks!

LGTM. @mattr-?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 8, 2013

Member

for a quick fix, this is fine. If you wanted to take this a step further, you could wrap the conditional in a method to improve the clarity of the code.

def extra_entry_text_empty?(post)
  post[:entry_text_more].nil? || post[:entry_text_more].strip.empty?
end

and replace the condition like so:

if extra_entry_text_empty?(post)
...
end
Member

mattr- commented Aug 8, 2013

for a quick fix, this is fine. If you wanted to take this a step further, you could wrap the conditional in a method to improve the clarity of the code.

def extra_entry_text_empty?(post)
  post[:entry_text_more].nil? || post[:entry_text_more].strip.empty?
end

and replace the condition like so:

if extra_entry_text_empty?(post)
...
end
@shigeya

This comment has been minimized.

Show comment
Hide comment
@shigeya

shigeya Aug 8, 2013

Contributor

I like that modification better, but only if you want to cost lines+methods for clarity. (besides the commit message's clarity). will change and push again later.

Contributor

shigeya commented Aug 8, 2013

I like that modification better, but only if you want to cost lines+methods for clarity. (besides the commit message's clarity). will change and push again later.

@shigeya

This comment has been minimized.

Show comment
Hide comment
@shigeya

shigeya Aug 8, 2013

Contributor

Updated.

Contributor

shigeya commented Aug 8, 2013

Updated.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 8, 2013

Member

I will always take clearer code over code that is less lines. Thanks for updating this! 😃

Member

mattr- commented Aug 8, 2013

I will always take clearer code over code that is less lines. Thanks for updating this! 😃

@shigeya

This comment has been minimized.

Show comment
Hide comment
@shigeya

shigeya Aug 8, 2013

Contributor

no problem. Looks like travis said OK.

Contributor

shigeya commented Aug 8, 2013

no problem. Looks like travis said OK.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 8, 2013

Member

Boom! LGTM.

Member

parkr commented Aug 8, 2013

Boom! LGTM.

mattr- added a commit that referenced this pull request Aug 9, 2013

Merge pull request #45 from shigeya/mt-entry_text_more-null-check
For MT: :entry_text_more attribute is nil or not before evaluate

@mattr- mattr- merged commit bb02ec7 into jekyll:master Aug 9, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Aug 9, 2013

@shigeya shigeya deleted the shigeya:mt-entry_text_more-null-check branch Aug 9, 2013

@shigeya

This comment has been minimized.

Show comment
Hide comment
@shigeya

shigeya Aug 9, 2013

Contributor

tnx

Contributor

shigeya commented Aug 9, 2013

tnx

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Oct 20, 2013

Contributor

Before I realized this had been fixed in master I'd gone with:

if post[:entry_text_more].to_s.strip.empty?

Before I realized this had been fixed in master I'd gone with:

if post[:entry_text_more].to_s.strip.empty?

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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