Skip to content

Conversation

@ashmaroli
Copy link
Member

Rationale

option is expected to be a Hash. Therefore option.to_s is never going to be empty since even {}.to_s equals "{}". Tracing the git history of the line of code shows that this is probably an error during merge-resolution.
I'm opting to use the more verbose options["tag"].nil? || options["tag"].empty? instead of options["tag"].to_s.empty? since this conditional is inside an #each block. Using the latter format would result in unnecessary allocations of "" when options["tag"] == nil

@ashmaroli ashmaroli requested a review from a team April 29, 2019 13:40
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! What do you think about using #fetch here instead?

@DirtyF
Copy link
Member

DirtyF commented May 2, 2019

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit e578203 into jekyll:master May 2, 2019
jekyllbot added a commit that referenced this pull request May 2, 2019
@ashmaroli ashmaroli deleted the nil-or-empty-tag-value branch May 2, 2019 06:00
@jekyll jekyll locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants