-
-
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: internal link misrepresented and misused #539
Fix: internal link misrepresented and misused #539
Conversation
also change usage of path to correctly handle the new possibility of nil
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 98.61% 98.61% +<.01%
==========================================
Files 30 30
Lines 1943 1951 +8
==========================================
+ Hits 1916 1924 +8
Misses 27 27
Continue to review full report at Codecov.
|
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.
Wow, some of this stuff, I don't even know how it was left in for so long... thank you.
@@ -49,7 +49,7 @@ def run | |||
next if @link.respond_to?(:rel) && @link.rel == 'dns-prefetch' | |||
add_to_external_urls(@link.href) | |||
next | |||
elsif !@link.internal? && [email protected]? | |||
elsif !@link.remote? && [email protected]? |
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 this be best as @link.internal?
(no !
)?
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 would. I tried it and it created some more errors failing. Looks to be some discrepancy there. I will give another go.
instead of forcing it to be nil
Tuns out #539 (comment) created quite the domino effect, it deserves another PR, if thats okay with you. |
Thanks! |
@@ -77,11 +79,11 @@ def path | |||
end | |||
|
|||
def hash | |||
parts.fragment unless parts.nil? | |||
parts&.fragment |
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 safe navigation fail for ruby < 2.3.0? and if so, is a minimum version of ruby specified anywhere? cc: @gjtorikian
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.
< 2.3 is EOL (in fact, 2.4 is EOL soon) and I don't support dead rubies.
I found some confusing code and followed it down the rabbit hole. I hope this will make the code more maintainable.