-
Notifications
You must be signed in to change notification settings - Fork 6
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
Refactoring and bug fixing #14
base: master
Are you sure you want to change the base?
Changes from 10 commits
cf01de3
fdc26d4
17fdd74
3775459
de7c573
3403478
fd16322
f3aa9d9
565c2c8
3ef88a5
5716c35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
--color |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
if ENV['HEADHUNTER'] == 'true' | ||
if ENV['HEADHUNTER'] == 'true' || ENV['RAILS_ENV'] == 'test' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why this was added, but it causes an infinite loop. I have to remove it to use this branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll have to investigate this myself. It's a long time ago already... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. infinite loop? Could you show me your command you ran? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it didn't matter what command I ran, it hung on precompiling assets which makes sense since this enables headhunter in all test environments regardless of whether or not you wanted to enable headhunter which would cause the assets to get precompiled (i'm guessing here, but that seems to be what happened) |
||
require 'headhunter/engine' | ||
require 'headhunter/css_hunter' | ||
require 'headhunter/css_validator' | ||
require 'headhunter/html_validator' | ||
require 'headhunter/rails' | ||
require 'headhunter/runner' | ||
require 'headhunter/runners/runner' | ||
require 'headhunter/runners/html_runner' | ||
require 'headhunter/runners/css_runner' | ||
end | ||
|
||
module Headhunter | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,17 +13,34 @@ def initialize | |
end | ||
|
||
def validate(uri, html) | ||
Dir.chdir(VALIDATOR_DIR) do | ||
raise "Could not find tidy in #{Dir.pwd}" unless File.exists? EXECUTABLE | ||
executable = %x[which #{EXECUTABLE}] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if executable.present? | ||
path = File.dirname(executable).strip | ||
executable = File.basename(executable).strip | ||
else | ||
path = VALIDATOR_DIR | ||
executable = EXECUTABLE | ||
end | ||
|
||
Dir.chdir(path) do | ||
fail "Could not find #{executable} in #{Dir.pwd}" unless File.exists? executable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [88/80] |
||
|
||
# tidy_version = `#{executable} -v` | ||
# puts "Using #{executable}: #{tidy_version}" | ||
|
||
# Docs for Tidy: http://tidy.sourceforge.net/docs/quickref.html | ||
stdin, stdout, stderr = Open3.popen3("#{EXECUTABLE} -quiet") | ||
stdin.puts html | ||
stdin.close | ||
stdout.close | ||
|
||
@responses << Response.new(stderr.read, uri) | ||
stderr.close | ||
begin | ||
stdin, stdout, stderr = Open3.popen3("#{executable} -quiet") | ||
stdin.puts html | ||
stdin.close | ||
stdout.close | ||
|
||
@responses << Response.new(stderr.read, uri) | ||
stderr.close | ||
rescue Encoding::UndefinedConversionError | ||
# not HTML, maybe something else (PDF, image, ...) | ||
end | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,21 +7,22 @@ def initialize(app, headhunter) | |
end | ||
|
||
def call(env) | ||
url = env['PATH_INFO'] || 'unknown' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I didn't find out where to get this from! 👍 |
||
response = @app.call(env) | ||
process(response) | ||
process(url, response) | ||
response | ||
end | ||
|
||
def process(rack_response) | ||
def process(url, rack_response) | ||
status, headers, response = rack_response | ||
|
||
if html = extract_html_from(response) | ||
@hh.process('unknown', html) | ||
@hh.process(url, html) | ||
end | ||
end | ||
|
||
def extract_html_from(response) | ||
response[0] | ||
response[0] if response.respond_to? :[] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I hit this bug, this is necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain this a bit better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a bug for a specific middleware request which triggered it, if it is not array-ish. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall exactly what it was, but they were sprockets assets that were returning. This should really check to make sure the thing is actually html I'd think. response header content type? |
||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it the following way:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was not sure If I should break the default options.