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

[WIP] #16 add geocode processor #88

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

Conversation

Libisch
Copy link
Contributor

@Libisch Libisch commented Aug 13, 2017

sync should add location field with lat/long (using geocoding of the place name)
#16

@Libisch Libisch requested a review from OriHoch August 13, 2017 15:07
Copy link
Contributor

@OriHoch OriHoch left a comment

Choose a reason for hiding this comment

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

👍 looks good, wrote some comments

@@ -124,6 +125,15 @@ entities-sync:
- run: dump.to_path
parameters:
out-path: ../data/clearmash/new-entities-sync
# dump geocode_location table
- run: dump.to_sql
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be better to have the sync processor update the geocode_entities table

@@ -149,3 +159,23 @@ entities-delete:
id-field: item_id
source: clearmash
delete-all-input: true

geocode:
Copy link
Contributor

Choose a reason for hiding this comment

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

this pipeline should be part of a common pipeline spec file, it's not specific to clearmash

"house_number": ["street_number", "house_number"],
"city": ["city"],
"country": ["country"],
"po_box": ["pob", "zipcode", "postal_code", "pob_postal_code"],}
Copy link
Contributor

Choose a reason for hiding this comment

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

this stuff is specific to budget key - it's irrelevant for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought maybe to leave it incase we'd like to extend location to other items in the future, like a detailed address of a photo subject (e.g painting, synagogue, person's residence and so on). But I guess it could be added if and when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

all this logic is specific only to open budget, even if we did need street number and this stuff - this code will not work..


# returns a location string to geocode by
# returns None if no suitable location string is found
def get_location_string(self, entity_details):
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is not needed, we get the location string as-is from the source data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change this processor to extend the BaseProcessor, like the rest?

Copy link
Contributor

Choose a reason for hiding this comment

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

would be better to have all processor use similar logic, but not critical

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.

2 participants