fixes for trac #4730 (also #4748) #216

Closed
wants to merge 42 commits into
from

3 participants

@erictheise

Modifications to

app/controllers/geocoder_controller.rb
app/helpers/geocoder_helper.rb
test/functional/geocoder_controller_test.rb

to "Support more syntax for lon/lat coordinates". Added tests, then added code and tests to get at the requests that are more clearly spelled out in #4748 than #4730. Went beyond the request so that latlon can be cut-and-paste from Wikipedia entries (and for that I added primes and double primes to the regexps).

Have not added support for UTM requested at the bottom of #4748.

erictheise added some commits Nov 10, 2012
@erictheise erictheise Fixing two typos 1d2581d
@erictheise erictheise Fixing typo and outdated information.
Changing 'posstible' to 'possible' in example.application.yml.
Updating wiki link and PostgreSQL version recommendation in
example.database.yml (8.3 will be EOLed in Feb 2013).
3983536
@erictheise erictheise Adding simple yml file for Travis Continuous Integration 1a87fae
@erictheise erictheise First draft of Travis CI YAML file 8fb8dc2
@erictheise erictheise Merge remote-tracking branch 'upstream/master' 59ab9f2
@erictheise erictheise Adding steps to before_script 5a8bb4c
@erictheise erictheise Create user correctly 4447114
@erictheise erictheise Create user correctly, continued 7c0c970
@erictheise erictheise Create user correctly, continued 1580067
@erictheise erictheise Create user correctly, continued ca40f8e
@erictheise erictheise Create user correctly, continued 82bea98
@erictheise erictheise Run the db migration cdff643
@erictheise erictheise Seems to work; adding rubies and status badge 33f366b
@erictheise erictheise Trying to install the quadtile functions aa74165
@erictheise erictheise Trying to install the quadtile functions, continued dc9f22e
@erictheise erictheise Trying to install the quadtile functions, continued fd39a0e
@erictheise erictheise merging upstream changes 27d3d49
@erictheise erictheise Re-enabling builds for 1.8.7 & 1.9.2 b47846c
@erictheise erictheise Merge remote-tracking branch 'upstream/master' 1fa7e5f
@erictheise erictheise Merge remote-tracking branch 'upstream/master' 5839086
@erictheise erictheise Merge remote-tracking branch 'upstream/master' 8ce529d
@erictheise erictheise Merge remote-tracking branch 'upstream/master' e1c0fa7
@erictheise erictheise Merge remote-tracking branch 'upstream/master' e801f51
@erictheise erictheise Merge branch 'master' of github.com:erictheise/openstreetmap-website 9163f73
@erictheise erictheise adding tests for existing behavior of geocoder search bd90a51
@erictheise erictheise tests and controller work to identify two general cases of degree/min…
…ute/second latlons; no calcs
ae503fc
@erictheise erictheise use * instead of ? for whitespace tests 82574df
@erictheise erictheise day's work; still need to test +/- NSEW 624ce8a
@erictheise erictheise found some bugs, more tests, moved tests to before_filter, verified c…
…onversions
382ebb7
@erictheise erictheise should be able to cut and paste out of wikipedia; adding single prime…
… alongside apostrophe, testing
d5bfc93
@erictheise erictheise adding double prime alongside quotation 8af6483
@openstreetmap-website
@tomhughes
OpenStreetMap on GitHub member

First impression is that this mostly looks like great work, so thanks for doing such a good job.

I'll have a detailed look at this in the next few days, but there are clearly some things here (like the travis config) that have nothing at all to do with fixing that bug and they should be removed from this pull please.

@erictheise

@tomhughes all the irrelevant changes have been removed from the pull request, and this should cover everything in #4748 up to the UTM example.

@tomhughes tomhughes commented on an outdated diff Feb 28, 2013
app/controllers/geocoder_controller.rb
@@ -312,4 +314,26 @@ def count_results(results)
def escape_query(query)
return URI.escape(query, Regexp.new("[^#{URI::PATTERN::UNRESERVED}]", false, 'N'))
end
+
+ def convert_latlon
+ @query = params[:query]
+
+ if latlon = @query.match(/^([NS])\s*(\d{1,3}\.?\d*)\W*([EW])\s*(\d{1,3}\.?\d*)$/).try(:captures) # [NSEW] decimal degrees
+ params[:query] = view_context.nsew_to_decdeg(latlon)
+ elsif latlon = @query.match(/^(\d{1,3}\.?\d*)\s*([NS])\W*(\d{1,3}\.?\d*)\s*([EW])$/).try(:captures) # degrees, decimal minutes [NSEW]
@tomhughes
OpenStreetMap on GitHub member
tomhughes added a line comment Feb 28, 2013

I don't think \.?\d* is right here (or on any of the others) as we shouldn't be matching the digits unless we saw the decimal point first, so surely the two should be grouped with the ? covering both?

In addition the comment on this line is wrong - looks like it has been cut and pasted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomhughes tomhughes and 1 other commented on an outdated diff Feb 28, 2013
app/controllers/geocoder_controller.rb
@@ -312,4 +314,26 @@ def count_results(results)
def escape_query(query)
return URI.escape(query, Regexp.new("[^#{URI::PATTERN::UNRESERVED}]", false, 'N'))
end
+
+ def convert_latlon
+ @query = params[:query]
+
+ if latlon = @query.match(/^([NS])\s*(\d{1,3}\.?\d*)\W*([EW])\s*(\d{1,3}\.?\d*)$/).try(:captures) # [NSEW] decimal degrees
+ params[:query] = view_context.nsew_to_decdeg(latlon)
+ elsif latlon = @query.match(/^(\d{1,3}\.?\d*)\s*([NS])\W*(\d{1,3}\.?\d*)\s*([EW])$/).try(:captures) # degrees, decimal minutes [NSEW]
+ params[:query] = view_context.nsew_to_decdeg(latlon)
+
+ elsif latlon = @query.match(/^([NS])\s*(\d{1,3})°?\s*(\d{1,3}\.?\d*)?['′]?\W*([EW])\s*(\d{1,3})°?\s*(\d{1,3}\.?\d*)?['′]?$/).try(:captures) # [NSEW] degrees, decimal minutes
@tomhughes
OpenStreetMap on GitHub member
tomhughes added a line comment Feb 28, 2013

What's with ['']? here? That is equivalent to '? which is a bit simpler... Same with [""]? in the later ones.

@erictheise
erictheise added a line comment Mar 1, 2013

The first checks for apostrophes (') and single primes (′), the second checks for quotation marks (") and double primes (″). A typographer would correctly use the prime characters, and, indeed, this is what Wikipedia does, so a cut and paste for any country latlon on Wikipedia will fail without them. test_primes_and_double_primes beginning on line 211 looks for these. Believe it's in the commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomhughes tomhughes and 1 other commented on an outdated diff Feb 28, 2013
app/helpers/geocoder_helper.rb
@@ -40,4 +40,41 @@ def describe_location(lat, lon, zoom = nil, language = nil)
"#{number_with_precision(lat, :precision => 3)}, #{number_with_precision(lon, :precision => 3)}"
end
end
+
+ def nsew_to_decdeg(captures)
@tomhughes
OpenStreetMap on GitHub member
tomhughes added a line comment Feb 28, 2013

Wouldn't it make more sense to put these methods in the controller? It's the only place they are used, their implementation is intimately tied to the regexs used in the controller method, and it will having to jump through hoops with view_context to call them.

Alternatively I would all the code in a library that can be included in the controller.

@erictheise
erictheise added a line comment Mar 1, 2013

You're right. I moved all the code into the controller and reverted geocoder_helper.rb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomhughes tomhughes and 1 other commented on an outdated diff Feb 28, 2013
app/helpers/geocoder_helper.rb
@@ -40,4 +40,41 @@ def describe_location(lat, lon, zoom = nil, language = nil)
"#{number_with_precision(lat, :precision => 3)}, #{number_with_precision(lon, :precision => 3)}"
end
end
+
+ def nsew_to_decdeg(captures)
+ begin
+ Float(captures[0])
+ captures[1].downcase != 's' ? lat = captures[0].to_f : lat = -(captures[0].to_f)
+ captures[3].downcase != 'w' ? lon = captures[2].to_f : lon = -(captures[2].to_f)
+ rescue
+ captures[0].downcase != 's' ? lat = captures[1].to_f : lat = -(captures[1].to_f)
+ captures[2].downcase != 'w' ? lon = captures[3].to_f : lon = -(captures[3].to_f)
+ end
+ return "#{number_with_precision(lat, :precision => 5)}, #{number_with_precision(lon, :precision => 5)}"
@tomhughes
OpenStreetMap on GitHub member
tomhughes added a line comment Feb 28, 2013

What's the logic behind using number_with_precision here rather than just putting out the whole value - it's only going to get converted back to a number anyway isn't it?

@erictheise
erictheise added a line comment Mar 1, 2013

Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomhughes tomhughes commented on the diff Feb 28, 2013
test/functional/geocoder_controller_test.rb
@@ -6,53 +7,217 @@ class GeocoderControllerTest < ActionController::TestCase
# test all routes which lead to this controller
def test_routes
assert_routing(
- { :path => "/geocoder/search", :method => :post },
@tomhughes
OpenStreetMap on GitHub member
tomhughes added a line comment Feb 28, 2013

Can we not make gratuitous changes to the quoting and indentation in this method please...

@erictheise
erictheise added a line comment Mar 1, 2013

Don't know how those snuck in. Sorry, reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomhughes
OpenStreetMap on GitHub member

Thanks for that. I've been through it properly now and put some comments in line.

@erictheise

@tomhughes I think I've addressed all your concerns. Definitely an improvement. Thanks for the feedback, let me know if you have any other questions/comments on the work.

@tomhughes
OpenStreetMap on GitHub member

Merged - thanks for helping out.

@tomhughes tomhughes closed this Mar 3, 2013
@erictheise erictheise deleted the unknown repository branch Mar 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment