Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

Kriechi
Copy link

@Kriechi Kriechi commented Jul 21, 2014

Here a list of things this PR addresses:

  • update and fix gem dependencies
  • use fail instead of raise for asserting
  • use tidy executable in current $PATH if available, else fallback to gem-included version
  • only check HTML-ish files, prevents errors for PDFs and other stuff
  • split HTML and CSS into separate runners - allows disabling of one part with HEADHUNTER_CSS=false
  • use the correct url for each validation if available

Let me know which parts you like, maybe you want to cherry-pick only certain commits - which is fine with me.

end

Dir.chdir(path) do
fail "Could not find #{executable} in #{Dir.pwd}" unless File.exists? executable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [88/80]

@@ -13,17 +13,26 @@ def initialize
end

def validate(uri, html)
Dir.chdir(VALIDATOR_DIR) do
raise "Could not find tidy in #{Dir.pwd}" unless File.exists? EXECUTABLE
tidy_path = %x[which #{EXECUTABLE}].strip
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%x-literals should be delimited by ( and )
Do not use %x unless the command string contains backquotes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I used backticks for executing system commands. What's better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you used Open3.popen3 - but I couldn't find a specific reason - so I changed it to use standard ruby invocation.

@jmuheim
Copy link
Owner

jmuheim commented Oct 20, 2014

Thank you, @aaronjensen! I will have to take an in-depth look at this. I'll try to do this soon.

Tell me, did you successfully use this gem? It was something like an experiment when I wrote it, and I didn't receive much feedback, so I didn't work on it anymore.

@aaronjensen
Copy link

Hi @jmuheim, I wasn't able to successfully use it, no. I had more luck with this pull request by @Kriechi, but it still caused several of my tests to fail. I'm really only interested in detecting unused css rules and I couldn't get deadweight working either so I tried this.

For now I used this: https://addons.mozilla.org/en-US/firefox/addon/dust-me-selectors/

but it would be great if there were an automated tool I could add on to my tests that'd give me a more accurate picture.

@jmuheim
Copy link
Owner

jmuheim commented Oct 20, 2014

Which PR by @Kriechi do you mean?

It's good to hear I'm not the only one interested in detecting unused CSS. Maybe we can give this project some new fire together?

I don't have much time at the moment, but I certainly will in a few weeks. Which project do you want to use it for? Is it public, so I could try to get it to run?

@Kriechi
Copy link
Author

Kriechi commented Oct 20, 2014

This PR. I created it a couple of months ago - so I'm not fully aware of the changes I made any more...
My main point for this PR was to split HTML and CSS detection because the HTML-framework I used was spitting out a lot of errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants