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

Refactor #6

Merged
merged 12 commits into from
Oct 14, 2013
Merged

Refactor #6

merged 12 commits into from
Oct 14, 2013

Conversation

benbalter
Copy link
Contributor

After digging around in the code a bit, it struck me that quadruple nested if loops may not be the bets way to go. This pull request refactors the link and images checkers for better relative URL support while also cleaning up the logic. As a result, several of the existing tests now properly fail, which required the creation of spec/html/proofer/fixtures/brokenInternalLink.html, spec/html/proofer/fixtures/folder/anchorLink.html, and spec/html/proofer/fixtures/notarealhash.html, the modification of spec/html/proofer/fixtures/brokenLinkWithNumber.html, and the removal of spec/html/proofer/fixtures/missingImageDirPrefix.html.

Specifically, it creates a Checkable class, which is extended and instantiated as Link and Image, with proper object oriented methods for the various checks.

Not sure if this is the best way to do things, and could probably use some clean up down the line, but believe the logic in the checks is now significantly clearer and was able to expose additional edge cases otherwise unhanded.

Thanks for a great, incredibly useful Gem. Looking forward to using it on my personal site.

@benbalter
Copy link
Contributor Author

Looks like tests are passing. Forgot to mention, this is based on #5.

Separately hope to refactor the tests as well.

@@ -1,7 +1,7 @@
PATH
remote: .
specs:
html-proofer (0.0.16)
html-proofer (0.0.16bb)
Copy link
Owner

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debugging artifact, re-bundled and pushed.

@gjtorikian
Copy link
Owner

<3 <3 <3 <3

Fucking open source, I love it.

gjtorikian added a commit that referenced this pull request Oct 14, 2013
@gjtorikian gjtorikian merged commit a7c3879 into gjtorikian:master Oct 14, 2013
@gjtorikian
Copy link
Owner

0.1.0 is released.

@benbalter benbalter deleted the refactor branch October 14, 2013 17:28
riccardoporreca added a commit to riccardoporreca/html-proofer that referenced this pull request Mar 14, 2023
* Most of the logic is legacy from the very early HTMLProofer, and is irrelevant not since `@filename` can only be the file where the link is defined, with `File.join` properly handling both same-directory, nested, and parent links, together with the ultimate `File.expand_path` in `absolute_path.
* The legacy logic comes pretty much from gjtorikian#6 and gjtorikian#23.
* This way we can also avoid `File.exist`, which can ultimately be delegated to checking the existence, not constructing the path.
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.

2 participants