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

URL: warn if key doesn't exist in url drop #5524

Merged
merged 4 commits into from Nov 9, 2016

Conversation

Projects
None yet
4 participants
@oe
Member

oe commented Nov 1, 2016

fixes #5332

the tests only cover not throwing a NoMethodError because i can't figure out how to capture the logged message to check if it logged the correct message (minitest's capture_io only works with kernel puts, weirdly enough)

@DirtyF DirtyF added the tests label Nov 2, 2016

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF
Member

DirtyF commented Nov 2, 2016

Show outdated Hide outdated lib/jekyll/url.rb
replacement = @placeholders.public_send(match.sub(":".freeze, "".freeze))
begin
replacement = @placeholders.public_send(match.sub(":".freeze, "".freeze))
rescue NoMethodError

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Instead of a begin .. rescue, what about checking for the key first, like

template.gsub(%r!:([a-z_]+)!) do |match|
  key = match.sub(":".freeze, "".freeze)
  raise NoMethodError, "No URL template key #{key} exists!" unless @placeholders.key?(key)
  if @placeholders[key].nil?
    "".freeze
  else
    self.class.escape_path(@placeholders[key])
  end
end.gsub(%r!//!, "/".freeze)
@parkr

parkr Nov 2, 2016

Member

Instead of a begin .. rescue, what about checking for the key first, like

template.gsub(%r!:([a-z_]+)!) do |match|
  key = match.sub(":".freeze, "".freeze)
  raise NoMethodError, "No URL template key #{key} exists!" unless @placeholders.key?(key)
  if @placeholders[key].nil?
    "".freeze
  else
    self.class.escape_path(@placeholders[key])
  end
end.gsub(%r!//!, "/".freeze)

This comment has been minimized.

@oe

oe Nov 2, 2016

Member

hmm, that makes more sense, yeah

@oe

oe Nov 2, 2016

Member

hmm, that makes more sense, yeah

This comment has been minimized.

@oe

oe Nov 2, 2016

Member

hmm, by using this approach, it looks like the URLDrop (at least the one i use in my tests) throws an unrelated error because it can't find fallback_data when using key? or #[]

@oe

oe Nov 2, 2016

Member

hmm, by using this approach, it looks like the URLDrop (at least the one i use in my tests) throws an unrelated error because it can't find fallback_data when using key? or #[]

This comment has been minimized.

@parkr

parkr Nov 2, 2016

Member

Ah, ok. No problem. Define def fallback_data in UrlDrop as an empty hash.

class UrlDrop
  private
  def fallback_data
    {}
  end
end
@parkr

parkr Nov 2, 2016

Member

Ah, ok. No problem. Define def fallback_data in UrlDrop as an empty hash.

class UrlDrop
  private
  def fallback_data
    {}
  end
end
fen
@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Nov 2, 2016

Member

updated according to @parkr's comments

Member

oe commented Nov 2, 2016

updated according to @parkr's comments

@parkr

parkr approved these changes Nov 2, 2016

LGTM!

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Nov 6, 2016

Member

@jekyll/core is this ok to merge? its been sitting for a while

Member

oe commented Nov 6, 2016

@jekyll/core is this ok to merge? its been sitting for a while

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 9, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Nov 9, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit f10c914 into jekyll:master Nov 9, 2016

1 of 2 checks passed

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

jekyllbot added a commit that referenced this pull request Nov 9, 2016

@oe oe deleted the oe:nomethoderror branch Nov 9, 2016

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