Skip to content

Conversation

@stevevance
Copy link
Contributor

The county property from a Mapzen result should become adminLevel 2, not the locality property. This brings it in line with Bing and ArcGIS Online (the other reverse geocoders I tested).

@willdurand willdurand requested a review from Nyholm April 23, 2018 00:22
@stevevance
Copy link
Contributor Author

I don't understand why the Travis CI build failed; I'm new to using this.

@Nyholm
Copy link
Member

Nyholm commented Jun 26, 2018

Great. This PR looks fine. The original author of this provider did not know if the county key (or it did not exits back then).

There are some test failing. Take the first failing as an example:

1) Geocoder\Provider\Mapzen\Tests\MapzenTest::testReverseWithRealCoordinates
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'England'
+'Lancaster'
/home/travis/build/geocoder-php/Geocoder/src/Provider/Mapzen/Tests/MapzenTest.php:104
/home/travis/build/geocoder-php/Geocoder/vendor/phpunit/phpunit/phpunit:53

On MapzenTest.php line 104 we have the line

$this->assertEquals('England', $result->getAdminLevels()->get(3)->getName());

But since you modified the array from $result->getAdminLevels() you also need to modify this line. It should work if you did ->get(4)


The next error is on MapzenTest.php:145. It assumes a size of the $result->getAdminLevels(). You should just modify that assertion.

You should do the same with the rest of the failures.

@jbelien
Copy link
Member

jbelien commented Sep 14, 2018

The Travis CI build fails because of those failed assertions :

  • MapzenTest.php#L104 : "Failed asserting that two strings are equal. (expected : 'England' - actual: 'Lancaster')"
  • MapzenTest.php#L145 : "Failed asserting that actual size 3 matches expected size 2."
  • MapzenTest.php#L192 : "Failed asserting that actual size 4 matches expected size 3."

Fix to pass Travis CI build.
Fix to pass Travis CI build.
@jbelien jbelien mentioned this pull request Sep 14, 2018
@jbelien
Copy link
Member

jbelien commented Sep 14, 2018

@stevevance I fixed the assertions. Travis CI build do not fail anymore.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both of you

@Nyholm Nyholm merged commit 3ac086b into geocoder-php:master Sep 14, 2018
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