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

Added an html validation check by looking at Nokogiri errors #125

Merged
merged 1 commit into from
Nov 4, 2014

Conversation

akoeplinger
Copy link
Contributor

I know you said in #30 you're not interested in validating HTML, but Nokogiri already looks at the markup and provides errors when parsing, so providing basic validation is quite easy.

This simple checker helped me catch a few mistakes in my site, so I thought I'd try and see if you find it useful now 😄

@gjtorikian
Copy link
Owner

All right, I am less grumpy about this now. Plus I like that it's a separate, non-default option.

Can you update the README with the new option? Everything else looks grand.

svg audio embed source track video)

def run
return unless @options[:validate_html]
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this superfluous?

Copy link
Owner

Choose a reason for hiding this comment

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

In what sense?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, because get_checks would've deleted it above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. If I want to run this check directly, I shouldn't need to know about the validate_html option, should I? If I'm calling it directly, the understanding would be that validate_html would be true. This is useful if I want to validate the HTML in some documents, but not all.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, you are correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the favicon code:

return unless @options[:favicon]

Do you still want me to remove it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think both should be removed

@akoeplinger
Copy link
Contributor Author

Added docs to readme, let me know if there's something else I can do.

@gjtorikian
Copy link
Owner

@akoeplinger I would agree that the favicon unless should be removed, too. Once that's done I'll merge. Thanks!

@akoeplinger
Copy link
Contributor Author

@gjtorikian sorry if that sounds pedantic, but I'd like to keep this PR focused on the new feature and not change unrelated code.
I can send a new PR that removes the favicon unless if you want.

@troyswanson
Copy link

😌 👍 🎉

Glad this is making its way in!

gjtorikian added a commit that referenced this pull request Nov 4, 2014
Added an html validation check by looking at Nokogiri errors
@gjtorikian gjtorikian merged commit 077ec93 into gjtorikian:master Nov 4, 2014
@akoeplinger akoeplinger deleted the html-validation branch November 4, 2014 12:11
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.

4 participants