Cleanup post_author logic
#113
Conversation
pathawks
force-pushed
the
pr/post_author
branch
from
662a77e to
4560388
Compare
|
Much cleaner. 👍 |
|
Could I get an extra set of eyes from @parkr or @benbalter please? |
| @@ -53,26 +53,12 @@ | |||
| <content type="html" xml:base="{{ post.url | prepend: url_base | xml_escape }}">{{ post.content | strip | xml_escape }}</content> | |||
|
|
|||
| {% assign post_author = post.author | default: post.authors[0] | default: site.author %} | |||
| {% assign post_author_email = false %} | |||
| {% assign post_author_uri = false %} | |||
| {% assign post_author = site.data.authors[post_author] | default: post_author %} | |||
There was a problem hiding this comment.
Should this be post_author_data not to be confused with post_author which is just the name?
There was a problem hiding this comment.
post.author can be a string or a hash. It's kind of a mess ¯_(ツ)_/¯
If we changed this line, we would also need to change the line above it to post_author_data, but then it would seem weird to {% assign post_author_data = site.data.authors[post_author_data] | default: post_author_data %}
I am open to suggestion though.
There was a problem hiding this comment.
Maybe if it is just a string, and not a reference, we assign it to author name.
There was a problem hiding this comment.
Wondering if, between this and Jekyll SEO Tag, it may make sense to have a shared jekyll-author gem, that would never be included directly (I guess it could?), but would handle author being a string, a hash, or a ref to _data, and would always return an object, maybe in Rubyland?
There was a problem hiding this comment.
@benbalter that's a great idea, but it would probably be best to start by releasing a patch that fixes the documented functionality first.
There was a problem hiding this comment.
Maybe if it is just a string, and not a reference, we assign it to author name.
I don't know of a way in Liquid to see what type a variable is.
it may make sense to have a shared
jekyll-authorgem
Totally agree, but as @GarthDB mentioned, let's get this bug fix out first.
There was a problem hiding this comment.
I'm on the road so I can't write out a full example, but just checking if the string is a key of the authors object and has a value, otherwise assume the string is a name.
There was a problem hiding this comment.
but it would probably be best to start by releasing a patch that fixes the documented functionality first.
Completely agree.
|
Excellent work! 👍 |
| {% assign post_author_email = false %} | ||
| {% assign post_author_uri = false %} | ||
| {% assign post_author = site.data.authors[post_author] | default: post_author %} | ||
| {% assign post_author_email = post_author.email | default: false %} |
There was a problem hiding this comment.
What's the rationale for having a false default, rather than nil?
There was a problem hiding this comment.
You are right, nil makes more sense.
pathawks
force-pushed
the
pr/post_author
branch
from
4560388 to
6e6b473
Compare
| {% assign post_author = site.data.authors[post_author] | default: post_author %} | ||
| {% assign post_author_email = post_author.email | default: nil %} | ||
| {% assign post_author_uri = post_author.uri | default: nil %} | ||
| {% assign post_author_name = post_author.name | default: post_author %} |
There was a problem hiding this comment.
Does this make more sense? (See also line 63)
There was a problem hiding this comment.
How does the behavior change if you remove the | default: nil?
There was a problem hiding this comment.
The behavior is the same either way, but I feel like including some sort of default is an acknowledgment that this may not be set, and that is ok.
If you do not think it makes the code easier to follow, I can remove it.
There was a problem hiding this comment.
The behavior is the same either way, but I feel like including some sort of default is an acknowledgment that this may not be set, and that is ok.
👍
|
@parkr @pathawks @benbalter while waiting for this I decided to work on a It should provide what would be needed by Is this something you'd be interested in using? If so, I'm happy to transfer the repo if needed and make pull requests to the 2 plugins. I did find out that the gem name |
|
Is this PR good to go, or is there still something left to modify? |
One style question, otherwise, 👍 (and agree we should merge this before looking into a more robust solution).
That's awesome. Thanks @GarthDB. One thing to note, that moves things from Liquid to Ruby, which isn't necessarily a bad thing, but makes things a bit trickier in terms of security. |
|
@benbalter is there a more secure way to do it that both jekyll-seo-tag and jekyll-feed could use? I figured jekyll hooks would be the best way to prep the data before either could use it. |
|
@jekyllbot: merge |
|
Thanks for the work @pathawks |
As pointed out by @GarthDB in #111, the way we were doing it previously was a mess that camouflaged bugs. This builds on PR, further simplifying things and hopefully giving bugs fewer places to hide.