Refactor Page#permalink method #4389

Merged
merged 1 commit into from Jan 22, 2016

Conversation

Projects
None yet
3 participants
@zsyed91
Contributor

zsyed91 commented Jan 22, 2016

  • Remove extra OR condition since a missing hash key will return a nil anyway and made it a 1 liner
  • Added a test to catch this nil condition since it was missing to begin with
  • Reduced line length in test_page.rb

Hope the change makes sense. I noticed there wasn't a test case to check for the sad path for .permalink. Does where I added this test make sense?

Remove extra OR condition since a missing hash key will return a nil …
…anyway. Added a test to catch this nil condition since it was missing to begin with. Reduced line length in test_page.rb
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 22, 2016

Member

This LGTM! Thanks for being so thorough, @zsyed91. 😄

Member

parkr commented Jan 22, 2016

This LGTM! Thanks for being so thorough, @zsyed91. 😄

@parkr parkr changed the title from Refactor page permalink method to Refactor Page#permalink method Jan 22, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 22, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Jan 22, 2016

@jekyllbot: merge +minor

jekyllbot added a commit that referenced this pull request Jan 22, 2016

@jekyllbot jekyllbot merged commit c42d27e into jekyll:master Jan 22, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Jan 22, 2016

@parkr parkr added the fix label Jan 22, 2016

@parkr parkr added this to the 3.1 milestone Jan 22, 2016

@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.