Use Octokit to fetch Gist content when passed a GItHub token #28

benbalter · 2015-11-30T21:30:48Z

Fixes #27

(Should work, but having trouble getting tests to run locally)

parkr · 2015-11-30T22:15:03Z

lib/jekyll-gist/gist_tag.rb

+      private
+
+      def code_from_api(gist_id, filename = nil)
+        return unless ENV["JEKYLL_GITHUB_TOKEN"]


Could this just be GITHUB_TOKEN?

I was going off the token Jekyll metadata used, so that you'd only have to set it once?

Good idea. I was thinking about other programs that use Octokit and what they use. Let's do JEKYLL_GITHUB_TOKEN for now – I think parity with jekyll-github-metadata for now makes sense. :) Thanks, Ben!

parkr · 2015-11-30T22:24:15Z

but having trouble getting tests to run locally

Sorry about this. Remove line 10 from spec/spec_helper.rb – that should help. That line swallowed errors.

benbalter · 2015-11-30T22:29:50Z

That line swallowed errors.

uninitialized constant Jekyll::Gist::GistTag::Octokit

Duh.

benbalter · 2015-11-30T23:01:32Z

Got tests to pass. Two changes to note, thinking through things:

If a token is supplied, and the API call fails, don't fallback to /raw (this wouldn't fix the DDoS problem described in the original issue if the API call fails due to, e.g., a 500 or a timeout).
If the API call fails, fail loudly (e.g., if it's a bad token, the Gist doesn't exist, etc.). Again, why make two bad calls when you can just make one.

parkr · 2015-12-01T00:13:21Z

why make two bad calls when you can just make one.

To make things work more seamlessly. If Octokit is enabled (via the env var), then failing loudly on a bad config makes sense. And failing if the resource doesn't exist makes sense in both cases.

CI is erroring out on Jekyll 2 and Ruby 1.9.3:

  1) Jekyll::Gist::GistTag valid gist with token with a filename produces the noscript tag
     Failure/Error: expect(output).to match(/<noscript><pre>puts &#39;hello world&#39;<\/pre><\/noscript>/)

       expected "<noscript><pre>puts 'hello world'</pre></noscript>\n<script src=\"https://gist.github.com/1342013.js?file=hello-world.rb\"> </script>\n\n" to match /<noscript><pre>puts &#39;hello world&#39;<\/pre><\/noscript>/
       Diff:
       @@ -1,2 +1,3 @@
       -/<noscript><pre>puts &#39;hello world&#39;<\/pre><\/noscript>/
       +<noscript><pre>puts 'hello world'</pre></noscript>
       +<script src="https://gist.github.com/1342013.js?file=hello-world.rb"> </script>

Looks like the dumb quotes aren't being escaped there.

benbalter · 2015-12-01T00:32:53Z

Looks like the output is different on Ruby 1.9? Should I make the test conditional? What's the "proper" way to do this?

parkr · 2015-12-01T01:03:24Z

Looks like the output is different on Ruby 1.9?

I think this is the problem 👆 The output shouldn't differ if you use a different Ruby. That's not The Jekyll Way ™️ . Ideally, we'd find a way to ensure it's the same output.

benbalter · 2015-12-01T17:36:53Z

The output shouldn't differ if you use a different Ruby.

Looks like it's a change in how CGI.escapeHTML behaves < Ruby 2.0..

Coded around it to keep the behavior the same in 87b93ed.

parkr · 2015-12-01T17:44:51Z

lib/jekyll-gist/gist_tag.rb

+      private
+
+      def code_from_api(gist_id, filename = nil)
+        client = Octokit::Client.new :access_token => ENV["JEKYLL_GITHUB_TOKEN"]


Can this be cached so it can be reused?

Done via d9c695f.

parkr · 2015-12-01T17:45:25Z

One optimization above, then LGTM. Thanks so much, @benbalter!

benbalter · 2015-12-01T17:51:34Z

💚

parkr · 2015-12-01T18:01:13Z

lib/jekyll-gist/gist_tag.rb

+      end
+
+      def client
+        @client ||= Octokit::Client.new :access_token => ENV["JEKYLL_GITHUB_TOKEN"]


Whoops, I'm awfully silly. Really sorry, Ben, I misspoke. When I asked about caching, my reasoning was that for each {% gist %}, a new GistTag instance is created and thus requires its own client. The Client won't change between various gist calls, so storing it so it's on the class rather than instance (or similar, store it anywhere you feel is appropriate) gives us a little speed boost and relieves some pressure on the garbage collector.

Does that make sense? Sorry I didn't communicate all that. Still figuring out the right balance between being concise (saves time) and communicating 100% of what I was thinking.

So this code would become:

def self.client @client ||= Octokit::Client.new #... end

and would be accessed via GistTag.client (i.e. at the class level).

Makes total sense. Didn't think through that. Does it need to be @@client?

Does it need to be @@client?

No, in the case of @client, it's an Instance variable on the Singleton. @@client is a class variable on GistTag, @client is an instance variable on the Singleton instance of GistTag. It's equivalent to:

class GistTag @client = Octokit::Client.new #... end

Ruby is hella weird but there are fun nuances like this that keep things interesting. 😉

Huh. 📚. Implemented via af85461.

Merge pull request 28

[ci skip]

use octokit to fetch gists when token present

6c2d000

benbalter self-assigned this Nov 30, 2015

parkr reviewed Nov 30, 2015
View reviewed changes

parkr added the enhancement label Nov 30, 2015

parkr added this to the 1.4.0 milestone Nov 30, 2015

before(:each)

15b2ee9

working tests

6401936

benbalter added 2 commits November 30, 2015 18:02

dont look for truncation

2d1b64e

document

63a6ef7

allways be backwards compatibling

87b93ed

benbalter added 4 commits December 1, 2015 12:37

Merge branch 'master' into api

b6eac05

missing end

60bca91

whitespace

91b8363

whitespace

32857aa

parkr reviewed Dec 1, 2015
View reviewed changes

benbalter added 2 commits December 1, 2015 12:46

cache client

d9c695f

balance whitespace

9153e43

parkr reviewed Dec 1, 2015
View reviewed changes

cache on the class, not the instance

af85461

parkr added a commit that referenced this pull request Dec 1, 2015

Merge pull request #28 from jekyll/api

cecdefe

Merge pull request 28

parkr merged commit cecdefe into master Dec 1, 2015

parkr deleted the api branch December 1, 2015 18:25

parkr added a commit that referenced this pull request Dec 1, 2015

Update history to reflect merge of #28

1a63a00

[ci skip]

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Octokit to fetch Gist content when passed a GItHub token #28

Use Octokit to fetch Gist content when passed a GItHub token #28

benbalter commented Nov 30, 2015

parkr Nov 30, 2015

benbalter Nov 30, 2015

parkr Nov 30, 2015

parkr commented Nov 30, 2015

benbalter commented Nov 30, 2015

benbalter commented Nov 30, 2015

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

parkr Dec 1, 2015

benbalter Dec 1, 2015

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

parkr Dec 1, 2015

benbalter Dec 1, 2015

parkr Dec 1, 2015

benbalter Dec 1, 2015

Use Octokit to fetch Gist content when passed a GItHub token #28

Use Octokit to fetch Gist content when passed a GItHub token #28

Conversation

benbalter commented Nov 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Nov 30, 2015

benbalter commented Nov 30, 2015

benbalter commented Nov 30, 2015

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Dec 1, 2015

benbalter commented Dec 1, 2015

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