Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

import all potentially useful fields from CSV in to model #402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

missinglink
Copy link
Member

imports some fields which are present in the CSV file but are not currently available on the model.

after I pushed this PR I realised that #320 covers some of the same territory.

additional fields imported:

  • UNIT -> setAddress( 'unit' );
  • CITY -> setMeta( 'city' );
  • DISTRICT -> setMeta( 'district' );
  • REGION -> setMeta( 'region' );

The metadata properties are useful downstream as 'hints' for the admin lookup service, this is particularly true in Australia where the CITY field in OA is better than the PIP locality.

I updated the tests and added a few more because the coverage wasn't great.

@orangejulius
Copy link
Member

unit is used in some queries already, so I think we do have to give this at least a little testing https://github.com/pelias/query/blob/master/layout/AddressesUsingIdsQuery.js#L44

That said I really like the idea of adding all this data and we should do it. However it might make sense to do it as part of a specific feature rather than in anticipation of some other PR using it.

Joxit added a commit that referenced this pull request Jan 20, 2021
missinglink pushed a commit that referenced this pull request Jan 21, 2021
* feat(import): add support for openaddresses geojsons format

* feat(import): map field used in #402

* chore(import): apply changes from code review
@NickStallman
Copy link

This branch has some simple conflicts compared to master.

Also should the default name also be modified to include the unit?
If you import 10 units for the same street number, they'd all have the same name if this isn't modified.
I'm not sure what the implications of that are either way.

        .setName( 'default', (record.NUMBER + ' ' + record.STREET) )

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.

None yet

3 participants