Optional config parameter for user-agent header, alias for OSMGeocoder #195

chriso0710 · 2016-10-27T16:31:11Z

Made the following changes regarding issue #194 :

new optional parameter Geokit::Geocoders::useragent
send user agent header for both net adapters
some small readme changes

Sorry for not providing any tests, but I was having some issues with

rake test

and as I am not familiar with vcr and simplecov, I was not very keen to debug the issue :-)

Additionally I created a new alias OsmGeocoder because of the following error in Multigeocoding with OSM:

E, [2016-10-27T17:37:51.610364 #9174] ERROR -- : Caught an error during Multi geocoding call: uninitialized constant Geokit::Geocoders::OsmGeocoder
E, [2016-10-27T17:37:51.610421 #9174] ERROR -- : /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:65:in `const_get'
/Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:65:in `geocoder'
/Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:32:in `block in do_geocode'
/Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:31:in `each'
/Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:31:in `do_geocode'
/Users/co/projects/geokit/lib/geokit/geocoders.rb:90:in `geocode'

…r because of class_name = "#{Geokit::Inflector.camelize(provider.to_s)}Geocoder"

houndci-bot · 2016-10-27T16:31:23Z

lib/geokit/net_adapter/typhoeus.rb

@@ -2,7 +2,8 @@ module Geokit
  module NetAdapter
    class Typhoeus
      def self.do_get(url)
-        ::Typhoeus.get(url.to_s)
+        Geokit::Geocoders.useragent ? headers = {'User-Agent' => Geokit::Geocoders.useragent} : headers = {}
+        ::Typhoeus.get(url.to_s, :headers => headers)


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2016-10-27T16:31:23Z

lib/geokit/net_adapter/typhoeus.rb

@@ -2,7 +2,8 @@ module Geokit
  module NetAdapter
    class Typhoeus
      def self.do_get(url)
-        ::Typhoeus.get(url.to_s)
+        Geokit::Geocoders.useragent ? headers = {'User-Agent' => Geokit::Geocoders.useragent} : headers = {}


Use the return of the conditional for variable assignment and comparison.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Can you address this comment, e.g. headers = Geokit::Geocoders.useragent ? {'User-Agent' => Geokit::Geocoders.useragent} : {}

houndci-bot · 2016-10-27T16:31:23Z

lib/geokit/net_adapter/net_http.rb

@@ -3,7 +3,8 @@ module NetAdapter
    class NetHttp
      def self.do_get(url)
        uri = URI(url)
-        req = Net::HTTP::Get.new(uri.request_uri)
+        Geokit::Geocoders.useragent ? headers = {'User-Agent' => Geokit::Geocoders.useragent} : headers = {} 


Use the return of the conditional for variable assignment and comparison.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Trailing whitespace detected.

coveralls · 2016-10-27T16:33:09Z

Coverage decreased (-0.07%) to 95.96% when pulling beb538a on chriso0710:master into ddd4130 on geokit:master.

mnoack · 2016-10-27T21:40:23Z

I mostly like the Pull Request, I think it just needs a test that verifies if Net::HTTP receives the user agent.

e.g. mock 'Net::HTTP::Get' class
and see if that mock recevies 'new' with the URI and headers containing the agent. And of course a test that if user agent isn't set that it receives empty headers.

chriso0710 · 2016-10-28T10:39:48Z

Thanks, I will give it a try with webmock next week...

…aram, reverse_geocode accepts :provider_order param

houndci-bot · 2016-11-11T23:55:48Z

test/test_useragent.rb

+    def test_typhoeus_set_to_default
+        stub_request(:get, URL).with(:headers => TYPHOEUSDEFAULTHEADERS.merge('User-Agent' => TYPHOEUSDEFAULT))
+
+        Geokit::Geocoders::useragent = nil


Do not use :: for method calls.

houndci-bot · 2016-11-11T23:55:48Z

test/test_useragent.rb

+    end
+
+    def test_typhoeus_set_to_default
+        stub_request(:get, URL).with(:headers => TYPHOEUSDEFAULTHEADERS.merge('User-Agent' => TYPHOEUSDEFAULT))


Use 2 (not 4) spaces for indentation.
Use the new Ruby 1.9 hash syntax.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:48Z

test/test_useragent.rb

+    def test_typhoeus_set_to_testagent
+        stub_request(:get, URL).with(:headers => TYPHOEUSDEFAULTHEADERS.merge('User-Agent' => TESTAGENT))
+
+        Geokit::Geocoders::useragent = TESTAGENT


Do not use :: for method calls.

houndci-bot · 2016-11-11T23:55:48Z

test/test_useragent.rb

+    end
+
+    def test_typhoeus_set_to_testagent
+        stub_request(:get, URL).with(:headers => TYPHOEUSDEFAULTHEADERS.merge('User-Agent' => TESTAGENT))


Use 2 (not 4) spaces for indentation.
Use the new Ruby 1.9 hash syntax.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:48Z

test/test_useragent.rb

+    def test_nethttp_useragent_set_to_default
+        stub_request(:get, URL).with(:headers => NETHTTPDEFAULTHEADERS.merge('User-Agent' => NETHTTPDEFAULT))
+
+        Geokit::Geocoders::useragent = nil


Do not use :: for method calls.

houndci-bot · 2016-11-11T23:55:49Z

test/test_useragent.rb

+    end
+
+    def test_nethttp_useragent_set_to_default
+        stub_request(:get, URL).with(:headers => NETHTTPDEFAULTHEADERS.merge('User-Agent' => NETHTTPDEFAULT))


Use 2 (not 4) spaces for indentation.
Use the new Ruby 1.9 hash syntax.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:49Z

test/test_useragent.rb

+    def test_nethttp_useragent_set_to_testagent
+        stub_request(:get, URL).with(:headers => NETHTTPDEFAULTHEADERS.merge('User-Agent' => TESTAGENT))
+
+        Geokit::Geocoders::useragent = TESTAGENT


Do not use :: for method calls.

houndci-bot · 2016-11-11T23:55:49Z

test/test_useragent.rb

+    URL                     = 'http://www.example.com'
+
+    def test_nethttp_useragent_set_to_testagent
+        stub_request(:get, URL).with(:headers => NETHTTPDEFAULTHEADERS.merge('User-Agent' => TESTAGENT))


Use 2 (not 4) spaces for indentation.
Use the new Ruby 1.9 hash syntax.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:49Z

test/test_useragent.rb

+    TYPHOEUSDEFAULT         = 'Typhoeus - https://github.com/typhoeus/typhoeus'
+    TYPHOEUSDEFAULTHEADERS  = {'Expect'=>''}
+    TESTAGENT               = 'MyAgent'
+    URL                     = 'http://www.example.com'


Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:49Z

test/test_useragent.rb

+    NETHTTPDEFAULTHEADERS   = {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3'}
+    TYPHOEUSDEFAULT         = 'Typhoeus - https://github.com/typhoeus/typhoeus'
+    TYPHOEUSDEFAULTHEADERS  = {'Expect'=>''}
+    TESTAGENT               = 'MyAgent'


Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+    NETHTTPDEFAULT          = 'Ruby'
+    NETHTTPDEFAULTHEADERS   = {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3'}
+    TYPHOEUSDEFAULT         = 'Typhoeus - https://github.com/typhoeus/typhoeus'
+    TYPHOEUSDEFAULTHEADERS  = {'Expect'=>''}


Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Surrounding space missing for operator =>.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+
+    NETHTTPDEFAULT          = 'Ruby'
+    NETHTTPDEFAULTHEADERS   = {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3'}
+    TYPHOEUSDEFAULT         = 'Typhoeus - https://github.com/typhoeus/typhoeus'


Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+class UserAgentTest < Test::Unit::TestCase
+
+    NETHTTPDEFAULT          = 'Ruby'
+    NETHTTPDEFAULTHEADERS   = {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3'}


Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Surrounding space missing for operator =>.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+
+class UserAgentTest < Test::Unit::TestCase
+
+    NETHTTPDEFAULT          = 'Ruby'


Use 2 (not 4) spaces for indentation.
Freeze mutable objects assigned to constants.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+require File.join(File.dirname(__FILE__), '../lib/geokit.rb')
+
+class UserAgentTest < Test::Unit::TestCase
+


Extra empty line detected at class body beginning.

houndci-bot · 2016-11-11T23:55:50Z

test/test_useragent.rb

+require 'test/unit'
+require 'webmock/test_unit'
+
+require File.join(File.dirname(__FILE__), '../lib/geokit.rb')


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

coveralls · 2016-11-11T23:57:22Z

Coverage increased (+0.02%) to 96.04% when pulling 3bd27f3 on chriso0710:master into ddd4130 on geokit:master.

coveralls · 2016-11-11T23:57:22Z

Coverage increased (+0.02%) to 96.04% when pulling 3bd27f3 on chriso0710:master into ddd4130 on geokit:master.

chriso0710 · 2016-11-12T00:03:51Z

Now here is my shot at webmock for testing the netadapters useragent. Hope it's ok this way. Please feel free to modify it.

I also fixed a bug with missing options param in bing geocoder, when using MultiGeocoder.
Added the :provider_order param in reverse_geocode. Same as in geocode.

mnoack · 2016-11-16T10:54:18Z

lib/geokit/geocoders/openstreetmap.rb

@@ -110,4 +110,6 @@ def self.set_bounds(result_json, loc)
      end
    end
  end
+
+  Geokit::Geocoders::OsmGeocoder = Geokit::Geocoders::OSMGeocoder


Why did you do this. I feel we're just adding another alias when people are already using OSM, if anything the class should reflect the file name (or file name reflect the class, e.g. OpenStreetMapGeocoder

Please see my first comment. Multigeocode throws an error in const_get, reason is the wrong class name in

class_name = "#{Geokit::Inflector.camelize(provider.to_s)}Geocoder"

E, [2016-10-27T17:37:51.610364 #9174] ERROR -- : Caught an error during Multi geocoding call: uninitialized constant Geokit::Geocoders::OsmGeocoder E, [2016-10-27T17:37:51.610421 #9174] ERROR -- : /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:65:in `const_get' /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:65:in `geocoder' /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:32:in `block in do_geocode' /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:31:in `each' /Users/co/projects/geokit/lib/geokit/multi_geocoder.rb:31:in `do_geocode' /Users/co/projects/geokit/lib/geokit/geocoders.rb:90:in `geocode'

coveralls · 2016-11-20T11:20:39Z

Coverage increased (+0.02%) to 96.04% when pulling d34386b on chriso0710:master into ddd4130 on geokit:master.

coveralls · 2016-11-20T11:20:39Z

Coverage increased (+0.02%) to 96.04% when pulling d34386b on chriso0710:master into ddd4130 on geokit:master.

coveralls · 2016-11-20T11:20:39Z

Coverage increased (+0.02%) to 96.04% when pulling d34386b on chriso0710:master into ddd4130 on geokit:master.

mnoack · 2017-03-26T12:35:24Z

@chriso0710 I've fixed tests in #202 so am closing this. Thanks very much for contributing to the fix.

chriso0710 · 2017-03-27T08:03:22Z

My pleasure, thanks for merging :-)

optional config parameter for user-agent header, alias for OSMGeocode…

beb538a

…r because of class_name = "#{Geokit::Inflector.camelize(provider.to_s)}Geocoder"

houndci-bot reviewed Oct 27, 2016

View reviewed changes

Testing Geokit::Geocoders::useragent, Bing geocode fix with options p…

3bd27f3

…aram, reverse_geocode accepts :provider_order param

houndci-bot reviewed Nov 11, 2016

View reviewed changes

mnoack reviewed Nov 16, 2016

View reviewed changes

fixed houndci-bot comment

d34386b

mnoack mentioned this pull request Mar 26, 2017

User agent #202

Merged

mnoack closed this Mar 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional config parameter for user-agent header, alias for OSMGeocoder #195

Optional config parameter for user-agent header, alias for OSMGeocoder #195

chriso0710 commented Oct 27, 2016

houndci-bot Oct 27, 2016

houndci-bot Oct 27, 2016

mnoack Nov 16, 2016

houndci-bot Oct 27, 2016

coveralls commented Oct 27, 2016 •

edited

mnoack commented Oct 27, 2016

chriso0710 commented Oct 28, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

houndci-bot Nov 11, 2016

coveralls commented Nov 11, 2016 •

edited

coveralls commented Nov 11, 2016

chriso0710 commented Nov 12, 2016

mnoack Nov 16, 2016

chriso0710 Nov 16, 2016

coveralls commented Nov 20, 2016 •

edited

coveralls commented Nov 20, 2016

coveralls commented Nov 20, 2016

mnoack commented Mar 26, 2017

chriso0710 commented Mar 27, 2017


		class UserAgentTest < Test::Unit::TestCase

		NETHTTPDEFAULT = 'Ruby'

		require File.join(File.dirname(__FILE__), '../lib/geokit.rb')

		class UserAgentTest < Test::Unit::TestCase

Optional config parameter for user-agent header, alias for OSMGeocoder #195

Optional config parameter for user-agent header, alias for OSMGeocoder #195

Conversation

chriso0710 commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 27, 2016 • edited

mnoack commented Oct 27, 2016

chriso0710 commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 11, 2016 • edited

coveralls commented Nov 11, 2016

chriso0710 commented Nov 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 20, 2016 • edited

coveralls commented Nov 20, 2016

coveralls commented Nov 20, 2016

mnoack commented Mar 26, 2017

chriso0710 commented Mar 27, 2017

coveralls commented Oct 27, 2016 •

edited

coveralls commented Nov 11, 2016 •

edited

coveralls commented Nov 20, 2016 •

edited