Skip to content

Conversation

@doclements
Copy link

No description provided.

Repository owner deleted a comment from coveralls Aug 20, 2017
Repository owner deleted a comment from coveralls Aug 20, 2017
Repository owner deleted a comment from coveralls Aug 20, 2017
Copy link
Owner

@perrygeo perrygeo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Some changes suggested below.

feature_stats['properties'] = feat['properties']
if preserve_ids:
if 'id' in feat['properties']:
feature_stats['id'] = feat['properties']['id']
Copy link
Owner

@perrygeo perrygeo Aug 20, 2017

Choose a reason for hiding this comment

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

Three suggestions for this section:

  1. We can simplify the branching logic here
if preserve_properties and 'properties' in feat:
    ...
  1. If the id is already part of properties, the preserve_ids implementation is superfluous. The true id, according to the GeoJSON spec, is at the top level of the feature.
if preserve_ids and 'id' in feat:
    feature_stats['id'] = feat['id']
  1. Because this code deals with massaging the output data, not directly with zonal stats itself, I'd like to move it to the bottom of the function. This should share code with the application of geojson_out (geojson_out is basically equivalent to preserve_properties + preserve_ids + preserve_geometry)

Copy link
Owner

Choose a reason for hiding this comment

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

Also, we'll need to add the new arguments to the function docstring.

Copy link
Author

Choose a reason for hiding this comment

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

github decided not to give any notifications for this so i've only just noticed the comments. I'll look into making the changes as discussed.

thanks

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants