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

Implement custom feature properties source #71

Closed
wants to merge 4 commits into from

Conversation

lrvdijk
Copy link
Contributor

@lrvdijk lrvdijk commented Aug 9, 2015

By default, when converting a django model instance with geometry
to GeoJSON, the "properties" field will be filled with the fields
defined in the corresponding serializer.

In this PR we separate the code a bit in individual methods,
which can be overridden by any subclass. In this way the user is
able to define their own source for the properties field of the
GeoJSON output.

Solves #70

By default, when converting a django model instance with geometry
to GeoJSON, the "properties" field will be filled with the fields
defined in the corresponding serializer.

In this commit we separate the code a bit in individual methods,
which can be overriden by any subclass. In this way the user is
able to define their own source for the properties field of the
GeoJSON output.
@lrvdijk
Copy link
Contributor Author

lrvdijk commented Aug 9, 2015

Sample usage:

class NetworkGeoSerializer(GeoFeatureModelSerializer):
    class Meta:
        model = models.Link
        geo_field = 'geo'
        auto_bbox = True

    def get_feature_properties(self, instance):
        return instance.metadata  # This is a PostgreSQL HStore field, which django maps to a dict

    def unformat_geojson(self, feature):
        attribs = {
            self.Meta.geo_field: feature["geometry"],
            "metadata": feature["properties"]
        }

        if self.Meta.bbox_geo_field and "bbox" in feature:
            attribs[self.Meta.bbox_geo_field] = Polygon.from_bbox(
                feature["bbox"])

        return attribs

@pglotov
Copy link
Contributor

pglotov commented Aug 9, 2015

Feels like this functionality belongs to ModelSerializer from DRF. GeoFeatureModelSerializer should just repackage fields, whatever they are that came after calling ModelSerializer. It is reasonable to have a situation like this arise with no regard to geolocation: serialize an object from a HStore as in the referred issue.

@lrvdijk
Copy link
Contributor Author

lrvdijk commented Aug 10, 2015

Hmm true, but this is still a little bit different, because we still have the geometry (and optionally the bounding box) which is part of the model/serializer.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling f2f4c01 on sh4wn:custom-feature-properties into b5947a9 on djangonauts:master.

@nemesifier
Copy link
Member

👍

I did not like the nested function declarations, a few months ago I also thought that they could make customization of the serializer more cumbersome.

For the moment I'll only be able to ask you 2 things:

  1. a few style changes for which i'll leave inline comments
  2. a few lines of documentation in the GeoFeatureModelSerializer section, in a new subsection right after Bounding Box: "auto_bbox" and "bbox_geo_field")

In a couple of weeks I'll test the feature properly and merge it if it's ready.

if not field.write_only:
value = field.get_attribute(instance)
ret["bbox"] = (value.extent if hasattr(value, 'extent') else
None)
Copy link
Member

Choose a reason for hiding this comment

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

generally I do not like to enforce strict line breaking because in instances like this one I have a hard time reading the code. Could you please put Noneon the line above?

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling f2f4c01 on sh4wn:custom-feature-properties into f58d9c5 on djangonauts:master.

@lrvdijk
Copy link
Contributor Author

lrvdijk commented Sep 4, 2015

Sorry for the delay, but those things you've mentioned should be fixed now! :)

@nemesifier
Copy link
Member

Thank you @sh4wn, I merged this manually in 5a82a00.

I needed to change a few bits in order to avoid losing some performance gains I managed to introduce in the latest versions, but the behaviour you introduced should be unchanged (except for the name of the method get_feature_properties which I renamed to get_properties for simplicity).

Please test 0.9.5 alpha in your app and report any feedback.

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.

None yet

4 participants