Fluid tag for displaying GitHub Gists with noscript tags in a Jekyll post

A while back I wrote a Fluid tag for Jekyll that inserts the contents of a GitHub Gist into the content of the page in ` tags for RSS readers. I've noticed a couple things I'm not happy with since posting it, but I am interested in the critique it will generate here on Stack Exchange!

(Obviously, some of the internals regarding methods required to make Liquid tags work (such as the parameters to initialize and having a render` method) aren't negotiable, but you get the idea.)

require 'digest/md5'
require 'net/https'
require 'uri'

module Jekyll
  class GistTag "
      string    += "#{code}

"
return string
end

def get_gist_url_for(gist, file)
"https://gist.github.com/raw/#{gist}/#{file}"
end

def cache_gist(gist, file, data)
file = get_cache_file_for gist, file
File.open(file, "w+") do |f|
f.write(data)
end
end

def get_cached_gist(gist, file)
return nil if @cache == false
file = get_cache_file_for gist, file
return nil unless File.exist?(file)
return File.new(file).read
end

def get_cache_file_for(gist, file)
bad_chars = /[^a-zA-Z0-9\-_\.]/
gist = gist.gsub bad_chars, ''
file = file.gsub bad_chars, ''
md5 = Digest::MD5.hexdigest "#{gist}-#{file}"
File.join @cache_folder, "#{gist}-#{file}-#{md5}.cache"
end

def get_gist_from_web(gist, file)
gist_url = get_gist_url_for(gist, file)
raw_uri = URI.parse(gist_url)
https = Net::HTTP.new(raw_uri.host, raw_uri.port)
https.use_ssl = true
https.verify_mode = OpenSSL::SSL::VERIFY_NONE
request = Net::HTTP::Get.new(raw_uri.request_uri)
data = https.request(request)
data = data.body
cache_gist(gist, file, data) unless @cache == false
data
end
end

class GistTagNoCache

A couple of mostly minor notes and improvements:

return "" unless @text =~ /([\d]*) (.*)/
gist, file = $1.strip, $2.strip

I generally try to avoid magic variables where ever possible. Using match and the resulting MatchData object tends to be less error prone.

code       = code.gsub "<", "<"

Is there a reason you only escape < characters? Unless I'm missing something, it seems preferable to use CGI.escapeHTML instead.

return string

It seems inconsistent to me that in some methods you have a return statement on the last line and in others you do not. I would always leave out the return statement on the last line as it is unnecessary.

return nil if @cache == false

It is pretty unidiomatic to use == with boolean literals - usually one would just negate the boolean instead of comparing to false. if !@cache or better unless @cache will read more natural to most people than if @cache == false.

File.new(file).read

You should never use File.new without storing its return value somewhere and then later calling close on it. As a matter of fact in most cases you should not use File.new at all, but rather File.open with a block, which closes the file automatically when the block's end is reached. The way you're doing it the file handle will remain open until the File object is garbage collected, which may be a while. This is a bad practice and can lead to subtle problems.

However in this case you don't even need File.open as there is already a method which does exactly what you need: File.read(file).

gist.gsub! /[^a-zA-Z0-9\-_\.]/, ''

You don't need to escape . as it has no special meaning inside brackets.

cache_gist(gist, file, data) unless @cache == false

This would really be a lot more readable without the explicit comparison and the double negation:

cache_gist(gist, file, data) if @cache

StackExchange Code Review Q#1268, answer score: 4