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

feat: enable specifying a root folder #540

Merged
merged 8 commits into from
Nov 17, 2019
Merged

feat: enable specifying a root folder #540

merged 8 commits into from
Nov 17, 2019

Conversation

Graborg
Copy link
Contributor

@Graborg Graborg commented Oct 1, 2019

Before: When running .check_file(...) on a file in a non-flat file-hierarchy, it is not possible to specify the root folder from which folder the file is being served. Instead the specified file's folder is assumed as root.
This leads to false positives when refering to a file outside of the specified file's folder.

After: enable specifying a root_dir from where the file is served, when a link begining with slash is found, html-proofer now checks the root_dir for any refered files, so all internal links can be resolved properly.

@Graborg Graborg requested a review from Floppy as a code owner October 1, 2019 15:37
@gjtorikian
Copy link
Owner

I like the idea, although it seems the test is failing. 🤔

@Graborg
Copy link
Contributor Author

Graborg commented Oct 2, 2019

Weird! Works on my machine. I will try to reproduce.

@Graborg
Copy link
Contributor Author

Graborg commented Oct 2, 2019

Tried booting up a ubuntu:16.04 container and following the exact footsteps of travis... Still runs smoothly.

Could you help me out with debugging this one? 😓

@gjtorikian
Copy link
Owner

@Graborg Can you set this PR to accept changes from the maintainer? I think I have the fix, I just want to see it in CI.

@gjtorikian
Copy link
Owner

Or I suppose I could just tell you the fix:

diff --git a/spec/html-proofer/links_spec.rb b/spec/html-proofer/links_spec.rb
index 3082671..e016145 100644
--- a/spec/html-proofer/links_spec.rb
+++ b/spec/html-proofer/links_spec.rb
@@ -81,7 +81,7 @@ describe 'Links test' do
 
   it 'succeeds for working internal-root-links pointing to other folder' do
     broken_root_link_internal_filepath = "#{FIXTURES_DIR}/links/working_root_link_internal.html"
-    proofer = run_proofer(broken_root_link_internal_filepath, :file, {:root_dir => "/app/spec/html-proofer/fixtures"})
+    proofer = run_proofer(broken_root_link_internal_filepath, :file, { root_dir: 'spec/html-proofer/fixtures' })
     expect(proofer.failed_tests).to eq []
   end

/app/ needs to be removed. (The code also should confirm to Rubocop's linting styles.)

@codecov-io
Copy link

codecov-io commented Oct 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@63663a4). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #540   +/-   ##
=========================================
  Coverage          ?   98.56%           
=========================================
  Files             ?       30           
  Lines             ?     1949           
  Branches          ?        0           
=========================================
  Hits              ?     1921           
  Misses            ?       28           
  Partials          ?        0
Impacted Files Coverage Δ
spec/html-proofer/links_spec.rb 99.28% <100%> (ø)
lib/html-proofer/element.rb 96.82% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63663a4...f447fc0. Read the comment docs.

@Graborg
Copy link
Contributor Author

Graborg commented Nov 14, 2019

Hi! Anything I can do to help get this PR merged? (Along with the others)

Thanks

@gjtorikian
Copy link
Owner

Thanks!

@gjtorikian gjtorikian merged commit 4a0b789 into gjtorikian:master Nov 17, 2019
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.

3 participants