Updates found in website upgrade: #24

wants to merge 5 commits into


None yet

3 participants


We had to make some changes to Tarantula to get things happy with the new website. Requesting review from @jondistad and @sumbach:

  • update spec helper to, uh, work (needed top level module or everything bombed)
  • use Mime::Type's built in html? method to see if html handlers should run
  • update specs appropriately
  • remove cruft from readme
Rob Sanheim ... added some commits Jan 3, 2012
Rob Sanheim and Jamie Kite Update old docs f573485
Rob Sanheim and Jamie Kite Get specs passing:
  Needed the top level module included for any specs to run
Rob Sanheim and Jamie Kite add one more test case just to be sure 81ea21e
Rob Sanheim and Jamie Kite Add more verbosity 14f2876
Rob Sanheim and Jamie Kite Change to correctly check content_type:
  Rails 3 now has a real Mime::Type object that it returns for
  content_type, so we ask that object if it is html.  Previously
  non html assets were being incorrectly marked as html.

This also closes #17.

@sumbach sumbach commented on the diff Jan 5, 2012
# some versions of Rails integration tests don't set content type
# so we are treating nil as html. A better fix would be welcome here.
- ((content_type =~ %r{^text/html}) != nil) || content_type == nil
+ return content_type.nil? || content_type.html?
sumbach Jan 5, 2012

Is content_type guaranteed to be an instance of Mime::Type?

cldwalker Feb 20, 2012

same concern here, wouldn't applying this kill rails2 support? are we fine with that? @jgkite, @rsanheim

jgkite Feb 20, 2012

Probably, but not applying it kills Rails 3 support. The problem we ran into here was that content_type is a Mime::Type object in Rails 3, which means you can't just compare it with a regex the way you're doing. This always returns true in Rails 3, since false != nil:

((content_type =~ %r{^text/html}) != nil)  || content_type == nil

...which means that everything — images, CSS files, etc — will get run against the HTML validator. The moral of the story is that while this pull request might not be a silver bullet, doing nothing means we're pointing Rails 3 projects at a fork until there's a fix.


This looks awesome, Jamie! Thanks for making the upstream fix (instead of monkey patching like someone else I know... :-)


I'm not sure we're even supposed to check Gemfile.lock into git (see http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/).


Fwiw, I've updated the requires + Gemfile.lock on master to have a passing test suite for travis. I'm not sure if travis needs Gemfile.lock.


I cherry-picked everything except fixing tests, added back Rails support to Response#html? and removed Gemfile.lock. And it's released. FYI, today is my "friday" (if you're wondering why all the activity)

@cldwalker cldwalker closed this Feb 20, 2012
@cldwalker cldwalker referenced this pull request Feb 20, 2012

The README is out of date #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment