-
Notifications
You must be signed in to change notification settings - Fork 135
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
Inject site.github
via :pre_render
step rather than :after_init
#238
Conversation
Thank you for tackling the linked issue this quickly, @parkr. After a cursory pass-through, I would like to leave some suggestions,though:
|
ab278bb
to
2cc6a8e
Compare
Jekyll 4 has a call to site.config.inspect, which renders all the values in site.config['github']. This causes all of the API calls to be resolved regardless of whether the user is using them. This patch should allow Jekyll 4 to call site.config.inspect without causing the large number of API calls.
2cc6a8e
to
8d43c2d
Compare
@ashmaroli Updated! Your review was quick – only 9 hours by my count! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM expect for two instances of nitpicking.
Either ways, feel free to address the comments / merge at your discretion.
add_title_and_description_fallbacks! | ||
add_url_and_baseurl_fallbacks! if should_add_url_fallbacks? | ||
end | ||
|
||
def inject_metadata!(payload) | ||
payload.site["github"] = github_namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that payload
is munged outside the #munge!
method, perhaps we can assign the metadata directly..
payload.site["github"] = case site.config["github"]
...
end
Not a blocker to merge current PR, though.
Co-authored-by: Ashwin Maroli <[email protected]>
@jekyllbot: merge +bug |
Jekyll 4 has a call to
site.config.inspect
, which renders all the values insite.config['github']
.This causes all of the API calls to be resolved regardless of whether the user is using them.
This patch should allow Jekyll 4 to call
site.config.inspect
without causing the large number of API calls.Fixes #237.