Add borough as a possible layer for geonames #612

Merged
merged 2 commits into from Aug 4, 2016

Conversation

Projects
None yet
3 participants
@avulfson17
Contributor

avulfson17 commented Aug 1, 2016

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Aug 1, 2016

Member

What, no tests? :)

Member

orangejulius commented Aug 1, 2016

What, no tests? :)

@avulfson17

This comment has been minimized.

Show comment
Hide comment
@avulfson17

avulfson17 Aug 2, 2016

Contributor

I think this tests it
test('alias layer mapping', function(t) {
t.deepEquals(type_mapping.layer_mapping.coarse,
[ 'continent', 'country', 'dependency', 'macroregion',
'region', 'locality', 'localadmin', 'macrocounty', 'county', 'macrohood',
'borough', 'neighbourhood', 'microhood', 'disputed' ]);
t.end();
});

Contributor

avulfson17 commented Aug 2, 2016

I think this tests it
test('alias layer mapping', function(t) {
t.deepEquals(type_mapping.layer_mapping.coarse,
[ 'continent', 'country', 'dependency', 'macroregion',
'region', 'locality', 'localadmin', 'macrocounty', 'county', 'macrohood',
'borough', 'neighbourhood', 'microhood', 'disputed' ]);
t.end();
});

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Aug 2, 2016

Member

Well, there's only one file changed, and it's the implementation code. The test didn't fail in the past, so it couldn't have covered the case you added :P

The test you copied is for the coarse alias. I think you want to add another test like this one for borough: https://github.com/pelias/api/blob/master/test/unit/sanitiser/_sources_and_layers.js#L54-L63

Member

orangejulius commented Aug 2, 2016

Well, there's only one file changed, and it's the implementation code. The test didn't fail in the past, so it couldn't have covered the case you added :P

The test you copied is for the coarse alias. I think you want to add another test like this one for borough: https://github.com/pelias/api/blob/master/test/unit/sanitiser/_sources_and_layers.js#L54-L63

@@ -53,7 +53,7 @@ module.exports.tests.no_errors = function(test, common) {
test('valid combination', function(t) {
var raw = {};
- var clean = { sources: ['geonames'], layers: ['macroregion'] };
+ var clean = { sources: ['geonames'], layers: ['borough'] };

This comment has been minimized.

@orangejulius

orangejulius Aug 3, 2016

Member

please duplicate this test, keeping the macroregion one, and adding a new borough one :P

@orangejulius

orangejulius Aug 3, 2016

Member

please duplicate this test, keeping the macroregion one, and adding a new borough one :P

@avulfson17 avulfson17 self-assigned this Aug 4, 2016

@avulfson17 avulfson17 removed the in progress label Aug 4, 2016

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Aug 4, 2016

Member

Test looks great, I updated the link to pelias/geonames#99 because it fixes the last step, and LGTM!

Member

orangejulius commented Aug 4, 2016

Test looks great, I updated the link to pelias/geonames#99 because it fixes the last step, and LGTM!

@dianashk

This comment has been minimized.

Show comment
Hide comment
@dianashk

dianashk Aug 4, 2016

Contributor

LGTM

Contributor

dianashk commented Aug 4, 2016

LGTM

@orangejulius orangejulius merged commit 134d11b into master Aug 4, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@orangejulius orangejulius removed the in review label Aug 4, 2016

@orangejulius orangejulius deleted the add-borough branch Aug 19, 2016

je-l pushed a commit to nlsfi/pelias-api that referenced this pull request Aug 31, 2017

Merge pull request #612 from pelias/add-borough
Add borough as a possible layer for geonames
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment