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

Add new only_4xx key to report just 4xx errors #96

Merged
merged 4 commits into from
Aug 22, 2014
Merged

Conversation

gjtorikian
Copy link
Owner

An ask from GitHub. A few changes here:

  • Dropping the as_link_array option. Proofer should be smart enough to know that the first arg is an array.
  • The option setup was convoluted, and is now cleaner
  • A new only_4xx option is possible, which, if set, ensures that Proofer only reports errors for 4xx level problems.

/cc @penibelst @aroben

@doktorbro
Copy link

Very nice.

``` bash
htmlproof '["http://github.com", "http://jekyllrb.com"]'
```

Copy link
Contributor

Choose a reason for hiding this comment

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

separate to this pr?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I might just revert the change to the command line tool. I don't feel comfortable with the change. I think it's a silly option for Ruby, though.

gjtorikian added a commit that referenced this pull request Aug 22, 2014
Add new `only_4xx` key to report just 4xx errors
@gjtorikian gjtorikian merged commit 7c8c8fc into master Aug 22, 2014
@gjtorikian gjtorikian deleted the report-only-404s branch August 22, 2014 21:50
@parkr
Copy link
Contributor

parkr commented Aug 22, 2014

Wait are you proofing google.com? Or are you making sure you can reach them? If the latter, I'd reconsider this.

I figure HTML::Proofer.new takes all directories (so either a string or an array of variable length) and checks those pages (even if a URL, in which case it does a wget/uri-open to get the html to parse).

I'd expect a different API for "can I access this website?" Like HTML::Proofer.ping("google.com").

@doktorbro
Copy link

I'd expect a different API for "can I access this website?"

I absolutely agree with @parkr. The tool is a HTML proofer, so only static files should be first class citizens.

@gjtorikian
Copy link
Owner Author

The functionality for arbitrary link checking has been around since last month.

I find it extremely useful in Rails apps for checking links in the UI, mailers, etc. In fact, both Pages and GitHub.com are starting to use Proofer for this very reason. The whole parallel link check set up with Typhoeus--checking as efficinetly and quickly as possible--is very useful for arbitrary links as well as those found in HTML docs.

Having said all that, though, I agree now there should've been a different API for this. I'll likely do that in future major revision since it's a breaking change.

The tool is a HTML proofer, so only static files should be first class citizens.

Indeed, but the link checking code is useful for arbitrary links and static files. Should it be put into its own gem? That way Proofer can use it for HTML files, and others can use it for arbitrary link checks. That makes sense to me, but I hope it doesn't become a maintenance nightmare.

@doktorbro
Copy link

Indeed, but the link checking code is useful for arbitrary links and static files.

If you want to do it with Proofer, then @parkr’s proposal about API change is reasonable:

# files
HTML::Proofer.file("./_site/**/*.html", {:only_4xx => true})

# arbitrary links
HTML::Proofer.ping(["http://example.com", "http://example.org"], {:only_4xx => true})

CLI with subcommands:

# files
htmlproof file "./_site/**/*.html" --only-4xx

# arbitrary links
htmlproof ping "http://example.com","http://example.org" --only-4xx

Should it be put into its own gem?

A gem is too much. I would talk with @typhoeus about adding a Typhoeus.check(urls, opts) method.

//cc @i0rek

@hanshasselberg
Copy link

Hey,

I am not sure what check is supposed to do. It sounds a bit like Typhoeus.head(url, options).success? is doing it already.

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