-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Fix the directory index test #105
Conversation
options = { :directory_index_file => "README.md" } | ||
link_pointing_to_directory = "#{FIXTURES_DIR}/links/link_pointing_to_directory.html" | ||
output = capture_stderr { HTML::Proofer.new(link_pointing_to_directory, options).run } | ||
output.should match "internally linking to folder-php/, which does not exist" |
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.
@gjtorikian Don’t you think a better message would be “internally linking to folder-php/README.md, which does not exist”?
//cc @lurch
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.
Wouldn't something like "internally linking to folder-php/, but README.md missing" be better, to differentiate it from the case where the source link is directly to folder-php/README.md
?
And on a side-note, how does/would html-proofer cope with the situation where you have multiple directory_index_file options? http://httpd.apache.org/docs/2.2/mod/mod_dir.html#directoryindex
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 like the specificity suggested by @lurch here: the directory exists, but the file within it does not.
And on a side-note, how does/would html-proofer cope with the situation where you have multiple directory_index_file options?
We'd probably make a separate PR to allow you to pass in an array of file types.
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.
the directory exists, but the file within it does not
That's an interesting point - does html-proofer display a different error message if you're linking to a directory, but the directory itself doesn't exist?
(obviously if the link doesn't have a trailing-slash, and the target doesn't exist, you'd have to assume the intention was to link to a file)
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.
It should...but now that I'm looking at the code, I see @penibelst's point. This distinction between folder or file is a bit tricky. We don't ever actually make a distinction, I don't think, about which is missing. We just say that the whole path is invalid. So whether folder-php
is there or not, or whether README.md
is there or not, a link to folder-php/README.md
with one of those missing causes a failure.
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 see.
I guess (theoretically at least) if you had a really deep file-path like some/crazy/long/deeply/nested/path/to/myfile.html
and you accidentally linked to some/crazy/long/deply/nested/path/to/myfile.html
, it might be nice to know which part of the directory-tree the first error occurred in? ;)
(but obviously you could only do that for local paths, it wouldn't generally work for external links)
Apologies if I'm sending things OffTopic!
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.
If someone links to a directory, she means a link to directory’s index file. Note, that the followlocation
is related to directory_index_file
.
@gjtorikian I leave to you, how the message should look like. For now this PR follows the current logic.
Travis fails because |
Fix the directory index test
Related to #104