Twingly::URL::Utilities.remove_scheme

[jage]

Test and code from Zambezi. A minor version bump is needed if merged.

Close #6

[walro]

LGTM, extensive test suite!

[dentarg]

What do you think about this?

$ git diff lib
diff --git a/lib/twingly/url/utilities.rb b/lib/twingly/url/utilities.rb
index cb0c33d..8e1800a 100644
--- a/lib/twingly/url/utilities.rb
+++ b/lib/twingly/url/utilities.rb
@@ -3,10 +3,10 @@ module Twingly
     module Utilities
       module_function

-      PROTOCOL_EXPRESSION = /^https?:/i
+      PROTOCOL_EXPRESSION = /https?:/i

       def remove_scheme(url)
-        url.gsub(PROTOCOL_EXPRESSION, '')
+        url.sub(PROTOCOL_EXPRESSION, '')
       end
     end
   end

(All tests pass)

[dentarg]

And what do you think of these additional tests?

$ git diff test
diff --git a/test/unit/utilities_test.rb b/test/unit/utilities_test.rb
index 94d5d04..32232dc 100644
--- a/test/unit/utilities_test.rb
+++ b/test/unit/utilities_test.rb
@@ -57,5 +57,19 @@ class TestUtilities < MiniTest::Unit::TestCase
       result = Twingly::URL::Utilities.remove_scheme(url)
       assert_equal '//www.thecloset.gr/people/bloggers-pick-ιωάννα-τσιγαρίδα', result
     end
+
+    should "only remove scheme from HTTP URL" do
+      url = 'http://feedjira.herokuapp.com/?url=http://developer.twingly.com/feed.xml'
+
+      result = Twingly::URL::Utilities.remove_scheme(url)
+      assert_equal '//feedjira.herokuapp.com/?url=http://developer.twingly.com/feed.xml', result
+    end
+
+    should "only remove scheme from HTTPS URL" do
+      url = 'https://feedjira.herokuapp.com/?url=https://signalvnoise.com/posts.rss'
+
+      result = Twingly::URL::Utilities.remove_scheme(url)
+      assert_equal '//feedjira.herokuapp.com/?url=https://signalvnoise.com/posts.rss', result
+    end
   end
 end

[walro]

What do you think about this?

Can't see where it'd go wrong, guess it's more performant too, which I like.

And what do you think of these additional tests?

Good ones!

[dentarg]

The Ruby Style Guide says that String#sub is faster than String#gsub.

[walro]

PROTOCOL_EXPRESSION = /https?:/i

I thought about this again, if we have

ftp://blabla.se?url=http://blabla.se it would not work as expected right? So I think we should keep the ^ around.

[dentarg]

True.

[dentarg]

Test case for that:

should "not remove scheme from non HTTP(S) URLs with parameter" do
  url = 'ftp://ftp.example.com/?url=https://www.example.com/'
  result = Twingly::URL::Utilities.remove_scheme(url)
  assert_equal 'ftp://ftp.example.com/?url=https://www.example.com/', result
end

[jage]

@dentarg I think the proposed changes are good! Keep the ^ in the regexp and add all the tests. If you have the code, can you commit it?

[jage]

LGTM, extensive test suite!

The tests are from Zambezi, just replaced def test_blabla to should "blabla" do :)

[dentarg]

If you have the code, can you commit it?

Done.

[jage]

👍