Skip to content
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

Merged
merged 2 commits into from
May 20, 2022

Conversation

parkr
Copy link
Member

@parkr parkr commented May 16, 2022

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.

Fixes #237.

@parkr parkr requested a review from ashmaroli May 16, 2022 21:15
@ashmaroli
Copy link
Member

ashmaroli commented May 17, 2022

Thank you for tackling the linked issue this quickly, @parkr.
Hope you don't mind waiting a couple of days for my review.

After a cursory pass-through, I would like to leave some suggestions,though:

  • For better readability, it'd be better if the hooks were contained under the plugin's namespace. Additionally, inject_payload! sounds wrong to me. We're technically injecting GH metadata into the payload So, how about inject_metadata! (or perhaps munge_payload! instead)?:
    module Jekyll
      module GitHubMetadata
        class SiteGitHubMunger
          # ...
    
          def inject_metadata!(payload)
            payload.site["github"] = github_namespace
          end
          alias_method :munge_payload!, :inject_metadata! # for illustration purpose only
    
          # ...
        end
    
        Jekyll::Hooks.register :site, :after_init do |site|
          SiteGitHubMunger.global_munger = SiteGitHubMunger.new(site)
          SiteGitHubMunger.global_munger.munge!
        end
    
        Jekyll::Hooks.register :site, :pre_render do |_site, payload|
          SiteGitHubMunger.global_munger.inject_metadata!(payload)
        end
    
      end # end GitHubMetadata
    end
  • The changes to Gemfile and script/test-site do not seem to have significance to this PR. Perhaps, you could commit them directly to main instead?
  • While you're updating the main branch, I think AppVeyor should stop testing with Ruby 2.4.

@parkr parkr force-pushed the dont-munge-site-github branch 2 times, most recently from ab278bb to 2cc6a8e Compare May 18, 2022 02:53
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.
@parkr
Copy link
Member Author

parkr commented May 18, 2022

@ashmaroli Updated! Your review was quick – only 9 hours by my count!

Copy link
Member

@ashmaroli ashmaroli left a 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
Copy link
Member

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.

lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved
Co-authored-by: Ashwin Maroli <ashmaroli@users.noreply.github.com>
@parkr
Copy link
Member Author

parkr commented May 20, 2022

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit a771d0d into main May 20, 2022
@jekyllbot jekyllbot deleted the dont-munge-site-github branch May 20, 2022 21:07
jekyllbot added a commit that referenced this pull request May 20, 2022
@jekyll jekyll locked and limited conversation to collaborators May 20, 2023
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.

API calls for each and every repository?
3 participants