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

DRYed GeoFeatureModelSerializer #76

Closed
wants to merge 1 commit into from

Conversation

pglotov
Copy link
Contributor

@pglotov pglotov commented Aug 20, 2015

This PR is an attempt to DRY out the GeoFeatureModelSerializer: main field loop is done in the DRF base class ModelSerializer, GeoFeatureModelSerializer only re-shuffles and adds geo specific fields. Also provides hooks to modify properties field.

@nemesifier
Copy link
Member

hi @pglotov, won't this conflict with #71?

@pglotov
Copy link
Contributor Author

pglotov commented Aug 21, 2015

It will, but i think this is a cleaner approach since we don't duplicate field loop already present in base class, just repackage base class result and add geo specific fields which base class is not aware how to deal with. @sh4wn what do you think? Are the hooks in this PR enough for customizing properties field?

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling b16f406 on pglotov:dry-serializer into f58d9c5 on djangonauts:master.

@nemesifier
Copy link
Member

I think #71 is a good approach, the naming of the methods are clear, it includes usage documentation as requested and I don't see a duplication of the field loop. If you see any way to improve #71 leave comments there please.

@nemesifier nemesifier closed this Sep 9, 2015
@pglotov
Copy link
Contributor Author

pglotov commented Sep 9, 2015

@nemesifier
Copy link
Member

Thank you @pglotov, we can keep track of this in a new issue, could you open it?

@openwisp openwisp locked and limited conversation to collaborators Sep 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants