Set title and description in dev #104

benbalter · 2017-07-27T14:54:54Z

A follow up to #101, the title and description were only being set in production, making local previewing difficult.

This PR updates the logic to set the title and description in dev by moving the conditional statement.

parkr · 2017-07-27T15:19:27Z

lib/jekyll-github-metadata/site_github_munger.rb

@@ -16,9 +16,8 @@ def munge!
        # This is the good stuff.
        site.config["github"] = github_namespace

-        return unless should_add_fallbacks?
-        add_url_and_baseurl_fallbacks!
        add_title_and_description_fallbacks!


I wonder if we should bail early if:

No auth token is present (if they rebuild frequently, they'll start getting 403 Rate Limit Exceeded errors which are confusing)

There is no internet connectivity

👍 Both are a new feature in themselves, and out of scope for this PR, but 👍 to tackling those in a subsequent pass.

parkr · 2017-07-27T15:20:50Z

spec/site_github_munger_spec.rb

@@ -83,6 +84,10 @@
    before(:each) do
      Jekyll::GitHubMetadata.client = Jekyll::GitHubMetadata::Client.new({ :access_token => "" })
    end
+    before do


Why not before(:each)? IIRC before is discouraged because of the random ordering of each test.

Not related to this PR, so I'll pull it out (debugging artifact).

benbalter · 2017-07-27T15:58:16Z

spec/test-site/index.html

@@ -9,7 +9,7 @@
 </head>
 <body>
    <div id="output"></div>
-    <script src="renderjson.js" type="text/javascript" charset="utf-8"></script>
+    <script src="{{ "renderjson.js" | absolute_url }}" type="text/javascript" charset="utf-8"></script>


Added this to ensure the internals that use url and baseurl don't blow up when either is a Proc.

parkr

Hooray for Value!

parkr · 2017-07-27T16:00:44Z

I'd give this a good whirl with your personal site then ship it.

benbalter · 2017-07-27T16:01:57Z

Also, to memorialize things here publicly, chatting with @parkr, I didn't like the idea that we'd be making API calls for every build, even if the user doesn't use site.title or site.description. By moving the calls to Value wrapped Procs, in theory, a site that has a baseurl set and doesn't call site.title, site.description, site.url or site.github.* should not make an API call.

parkr · 2017-07-27T18:11:05Z

Thanks!

If you give the proc an arity of 2, it'll pass you the right repo too! First argument is the Client, second is the Repository 😄

benbalter · 2017-07-27T20:57:30Z

@parkr this could use a second set of 👀 now, but otherwise should be good to go. Per our discussion:

Created RepositoryFinder to centralize nwo logic
Centralized and memoized site and repository on the GitHubMetadata class
Added unit tests for each expected key/value in MetadataDrop
Added a test to ensure we're testing all values in MetadataDrop
Value now forwards +, and eql? to the value to avoid to_s calls

As a result, when called locally, by default, the plugin should make no external API calls. Only if, for example, site.title is called, and is not defined, or site.github.* is called, will it make an API call.

parkr

Great work! Thank you for this. I have just a few suggestions & requests. 😸

parkr · 2017-08-01T15:54:18Z

lib/jekyll-github-metadata.rb

@@ -61,9 +62,18 @@ def client
        @client ||= Client.new
      end

+      def repository
+        @repository ||= GitHubMetadata::Repository.new(nwo).tap do |repo|


Exactly what I was looking for, nice 👍

parkr · 2017-08-01T15:55:28Z

lib/jekyll-github-metadata.rb

+      end
+
+      def nwo
+        @nwo ||= GitHubMetadata::RepositoryFinder.nwo


For anything state-related, it would be nice to use an instance of RepositoryFinder here. It'd be much the same, but the state is thrown away once the nwo is discovered.

@nwo ||= Jekyll::GitHubMetadata::RepositoryFinder.new(site).nwo

If we did that, JekyllGitHubMetadata would have to know (or be passed) the site variable (which you seemed to indicate the opposite below).

My struggle is that it's too easy for this data to get all out of sync. I want the behavior that if GitHubMetadata.site changes, that GitHubMetadata.repository changes with it.

module GitHubMetadata def self.site repository_finder.site end def self.site=(new_site) reset! @repository_finder = GitHubMetadata::RepositoryFinder.new(new_site) end def self.repository @repository ||= GitHubMetadata::Repository.new(repository_finder.nwo).tap do |repo| Jekyll::GitHubMetadata.log :debug, "Generating for #{repo.nwo}" end end private attr_reader :repository_finder end end

This enforces a safe top-level environment: when site is set to some other value, it resets everything and creates a new repository_finder instance with the new site.

parkr · 2017-08-01T15:57:26Z

lib/jekyll-github-metadata/repository_finder.rb

+module Jekyll
+  module GitHubMetadata
+    class RepositoryFinder
+      class << self


Let's use an instance here. Might be even easier to test that way since the site would just be an instance variable; easier to remember to pass a required parameter to .new than it is to set GitHubMetadata.site before the test executes.

parkr · 2017-08-01T16:00:05Z

lib/jekyll-github-metadata/site_github_munger.rb

-      attr_reader :site
+      extend Forwardable
+
+      def_delegators :"Jekyll::GitHubMetadata", :site, :repository


Do these need to be public?

site was already public, but made repo private via 66f4b4a.

parkr · 2017-08-01T16:01:06Z

lib/jekyll-github-metadata/site_github_munger.rb

@@ -35,13 +36,14 @@ def github_namespace
      end

      def drop
-        @drop ||= MetadataDrop.new(site)
+        @drop ||= MetadataDrop.new


This makes me feel woozy. I wonder if keeping the site on the global GitHubMetadata is a bad idea? I'm ok with client and repository at that level, but the whole site object scares me.

Thoughts on a better solution? We need site in order to derive the nwo to build repository.

parkr · 2017-08-01T16:01:19Z

lib/jekyll-github-metadata/site_github_munger.rb

      end

      # Set `site.url` and `site.baseurl` if unset.
      def add_url_and_baseurl_fallbacks!
-        site.config["url"] ||= repository.url_without_path
-        site.config["baseurl"] = repository.baseurl if should_set_baseurl?
+        site.config["url"] ||= Value.new("url", proc { |_c, r| r.url_without_path })


parkr · 2017-08-01T16:04:40Z

lib/jekyll-github-metadata/value.rb

@@ -3,6 +3,9 @@
 module Jekyll
  module GitHubMetadata
    class Value
+      extend Forwardable
+      def_delegators :render, :+, :to_s, :to_json, :eql?


Clever. Can you memoize render so it doesn't render on each call? Or use :value instead of :render.

I'd also like to have a test for each of these – it can be really basic, but I just want to ensure they don't break.

Why #+?

Per Object#hash, if you specify #eql? you must also specify #hash:

Generates an Integer hash value for this object. This function must have the property that a.eql?(b) implies a.hash == b.hash.

Memoized via 7027532.

Why #+?

Workaround so we don't need to backport jekyll/jekyll#6253.

Per Object#hash, if you specify #eql? you must also specify #hash:

Done via f0662b0.

I'd also like to have a test for each of these – it can be really basic, but I just want to ensure they don't break.

added via 4ea6404

parkr · 2017-08-01T16:14:12Z

lib/jekyll-github-metadata/value.rb

-        render.to_s
-      end
-
-      def to_json(*)


Unfortunately, we can't just alias this method to #render. to_json must be able to accept a state parameter. We do that in Jekyll::Drops::Drop to great effect. (See how state affects things in DocumentDrop#hash_for_json.)

def to_json(state = nil) require "json" JSON.generate(value, state) end

Because it's def_delegators (plural), I believe it will delegate Value#to_json to calling to_json on the result of #render, meaning it should accept the second argument so long as the result of #render does.

…ts constructor.

parkr · 2017-08-07T20:11:14Z

@jekyllbot: merge +minor

set title and description in dev

b380901

benbalter requested a review from parkr July 27, 2017 14:54

parkr approved these changes Jul 27, 2017

View reviewed changes

benbalter added 3 commits July 27, 2017 11:31

use before each in integration spec

5580cdd

url, title, and description should be Values to save an API call in dev

94c5cce

make baseurl a Proc for consistency

dfd2d66

benbalter commented Jul 27, 2017

View reviewed changes

parkr approved these changes Jul 27, 2017

View reviewed changes

benbalter added 10 commits July 27, 2017 14:48

pass repo to proc

35ea54d

centralize site and nwo logic with RepositoryFinder

5afbd58

name the Values

87e3068

fix for absolute_url erroring due to + not being defined for site.url

99cf589

dont overwrite site if site isnt given

ba2936b

dont pass site to metadata drop

201340b

no need to nil repository again, its already reset

2f8064f

delegate eql? to avoid to_s calls

01533f2

remove debug output

040e7fd

need to actually call site.github.url to raise the error

36d00d0

Merge branch 'master' into set-title-in-dev

5c0640c

parkr requested changes Aug 1, 2017

View reviewed changes

benbalter and others added 6 commits August 1, 2017 12:34

make repository private on site munger

66f4b4a

memoize Value#render

7027532

delegate hash

f0662b0

simplify Value#render

99fa951

add tests for Value delegators

4ea6404

Update RepositoryFinder to have instance methods and take a site in i…

34330ae

…ts constructor.

MetadataDrop: #initialize should take 1 argument

2c4518d

parkr approved these changes Aug 7, 2017

View reviewed changes

jekyllbot merged commit 6b53042 into master Aug 7, 2017

jekyllbot added the enhancement label Aug 7, 2017

jekyllbot deleted the set-title-in-dev branch August 7, 2017 20:11

jekyllbot added a commit that referenced this pull request Aug 7, 2017

Update history to reflect merge of #104 [ci skip]

b7195ed

jekyll locked and limited conversation to collaborators Apr 28, 2019

jekyllbot added the frozen-due-to-age label Apr 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set title and description in dev #104

Set title and description in dev #104

benbalter commented Jul 27, 2017

parkr Jul 27, 2017

benbalter Jul 27, 2017

parkr Jul 27, 2017

benbalter Jul 27, 2017

benbalter Jul 27, 2017

parkr left a comment

parkr commented Jul 27, 2017

benbalter commented Jul 27, 2017

parkr commented Jul 27, 2017

benbalter commented Jul 27, 2017

parkr left a comment

parkr Aug 1, 2017

parkr Aug 1, 2017

benbalter Aug 1, 2017

parkr Aug 3, 2017

parkr Aug 1, 2017

parkr Aug 1, 2017

benbalter Aug 1, 2017

parkr Aug 1, 2017

benbalter Aug 1, 2017 •

edited

parkr Aug 1, 2017

parkr Aug 1, 2017

benbalter Aug 1, 2017

benbalter Aug 1, 2017

parkr Aug 1, 2017

benbalter Aug 1, 2017

parkr commented Aug 7, 2017

Set title and description in dev #104

Set title and description in dev #104

Conversation

benbalter commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr left a comment

Choose a reason for hiding this comment

parkr commented Jul 27, 2017

benbalter commented Jul 27, 2017

parkr commented Jul 27, 2017

benbalter commented Jul 27, 2017

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbalter Aug 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Aug 7, 2017

benbalter Aug 1, 2017 •

edited