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

Site#configure_theme: do not set theme unless it's a string #5189

Merged
merged 5 commits into from Aug 30, 2016

Conversation

Projects
None yet
6 participants
@parkr
Member

parkr commented Aug 3, 2016

Some previous ad-hoc 'themes' used this configuration option to store a hash of values.
In that case, we should simply pretend we have no theme.

Fixes #5163.

/cc @benbalter @emico7 @getaaron @wreichard

Site#configure_theme: do not set theme unless it's a string
Some previous ad-hoc 'themes' used this configuration option to store a hash of values.
In that case, we should simply pretend we have no theme.

@parkr parkr added the fix label Aug 3, 2016

@parkr parkr added this to the 3.2.1 milestone Aug 3, 2016

@getaaron

This comment has been minimized.

Show comment
Hide comment
@getaaron

getaaron Aug 3, 2016

Would it be better to inform the developer of the new use of theme instead of silently falling back to the old behavior? I sort of prefer the current crash to this because you know there's a problem. I suggest raising a helpfully worded exception if the object exists but is not a string.

If you disagree and still want to support the old behavior, then this solution looks good to me.

Thanks for fixing so quickly. 👍

getaaron commented Aug 3, 2016

Would it be better to inform the developer of the new use of theme instead of silently falling back to the old behavior? I sort of prefer the current crash to this because you know there's a problem. I suggest raising a helpfully worded exception if the object exists but is not a string.

If you disagree and still want to support the old behavior, then this solution looks good to me.

Thanks for fixing so quickly. 👍

Show outdated Hide outdated lib/jekyll/site.rb
self.theme = Jekyll::Theme.new(config["theme"])
else
Jekyll.logger.warn "Theme:",
"value of 'theme' in config should be String, but got #{config["theme"].class}"

This comment has been minimized.

@parkr

parkr Aug 7, 2016

Member

What do you think about this? /cc @benbalter @pathawks

@parkr

parkr Aug 7, 2016

Member

What do you think about this? /cc @benbalter @pathawks

This comment has been minimized.

@pathawks

pathawks Aug 7, 2016

Member

LGTM

This comment has been minimized.

This comment has been minimized.

@benbalter

benbalter Aug 8, 2016

Contributor

👍

@benbalter

benbalter Aug 8, 2016

Contributor

👍

@parkr parkr added the ux label Aug 7, 2016

@envygeeks envygeeks modified the milestones: 3.2.2, 3.2.1 Aug 12, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 22, 2016

Contributor

LGTM.

Contributor

benbalter commented Aug 22, 2016

LGTM.

@parkr parkr referenced this pull request Aug 29, 2016

Closed

Undefined method `downcase' error with the latest version #5163

3 of 10 tasks complete

@parkr parkr self-assigned this Aug 29, 2016

Show outdated Hide outdated lib/jekyll/site.rb
@@ -424,7 +424,7 @@ def configure_plugins
private
def configure_theme
self.theme = nil
self.theme = Jekyll::Theme.new(config["theme"]) if config["theme"]
self.theme = Jekyll::Theme.new(config["theme"]) if config["theme"].is_a?(String)

This comment has been minimized.

@envygeeks

envygeeks Aug 29, 2016

Contributor
self.theme = if ...
end

Will result in the same thing as those two lines. That'll rely on you using the if on the setter rather than setting if the condition is met. However it's a little more optimized that way and results in less messages.

@envygeeks

envygeeks Aug 29, 2016

Contributor
self.theme = if ...
end

Will result in the same thing as those two lines. That'll rely on you using the if on the setter rather than setting if the condition is met. However it's a little more optimized that way and results in less messages.

Show outdated Hide outdated lib/jekyll/site.rb
@@ -424,7 +424,14 @@ def configure_plugins
private
def configure_theme
self.theme = nil
self.theme = Jekyll::Theme.new(config["theme"]) if config["theme"]
return unless config["theme"]

This comment has been minimized.

@envygeeks

envygeeks Aug 29, 2016

Contributor

You can optimize this:

private
def configure_theme
  self.theme = 
    if config["theme"].is_a?(String)
      Jekyll::Theme.new(config[
        "theme"
      ])
    else
      Jekyll.logger.warn "Theme:", "value of 'theme' in config should be "
        "String, but got #{config["theme"].class}"
      nil
    end
end
@envygeeks

envygeeks Aug 29, 2016

Contributor

You can optimize this:

private
def configure_theme
  self.theme = 
    if config["theme"].is_a?(String)
      Jekyll::Theme.new(config[
        "theme"
      ])
    else
      Jekyll.logger.warn "Theme:", "value of 'theme' in config should be "
        "String, but got #{config["theme"].class}"
      nil
    end
end

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

That prints the warning if you don't set theme at all, which is not desired. i'll update it.

@parkr

parkr Aug 30, 2016

Member

That prints the warning if you don't set theme at all, which is not desired. i'll update it.

parkr added some commits Aug 30, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

Can I get another LGTM, please? /cc @benbalter

Member

parkr commented Aug 30, 2016

Can I get another LGTM, please? /cc @benbalter

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 30, 2016

Contributor

LGTM.

Contributor

benbalter commented Aug 30, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Aug 30, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 6a34966 into master Aug 30, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Aug 30, 2016

@jekyllbot jekyllbot deleted the skip-theme-if-not-string branch Aug 30, 2016

jekyllbot added a commit that referenced this pull request Aug 30, 2016

@parkr parkr modified the milestones: 3.2.2, 3.3 Sep 20, 2016

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