-
Notifications
You must be signed in to change notification settings - Fork 208
[fix] Make GeoJSON output valid by transforming to WGS84 #326
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
Conversation
@auvipy thank you for your review. I have noted two questions of mine at the top. What are your thoughts on these? |
Answers:
|
I've added tests for the |
README.rst
Outdated
@@ -97,6 +97,10 @@ This field takes three optional arguments: | |||
- ``auto_bbox``: If ``True``, the GeoJSON object will include | |||
a `bounding box <https://datatracker.ietf.org/doc/html/rfc7946#section-5>`_, | |||
which is the smallest possible rectangle enclosing the geometry. | |||
- ``transform`` (defaults to ``4326``): If ``None`` (or the input geometry does not have | |||
a SRID), the GeoJSON's coordinates will not be transformed. If any other `spatial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If any other" this line is not clear, can you please rephrase it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rephrased these docs after setting the default value to None
. I hope it is easier to understand now. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @auvipy
This is to be backwards-compatible with previous versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me. but I want to wait for other maintainers to look into it. you might need to push again to make the CI green again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StefanBrand can you please merge with the latest master and run openwisp-qa-format
again?
Make sure to have the dev version of openwisp-utils[qa] (uninstall and resinstall if necessary to update the openwisp-qa-format
script), eg:
pip uninstall openwisp-utils
pip install "openwisp-utils[qa] @ https://github.com/openwisp/openwisp-utils/tarball/1.2"
For the rest it looks good to me as well, thank you @StefanBrand @auvipy!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems we also need to fix the merge conflicts as well!
The conflicts are likely due to the recent black formatter changes, running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the conflcits and qa issues, I am merging, thanks everyone who contributed to this @StefanBrand @auvipy 👏
Checklist
Reference to Existing Issue
Closes #188.
Description of Changes
If the database column has a SRID other than 4326 (WGS84), the geometry and---if configured---its
bbox
is transformed to 4326 to conform with the GeoJSON spec.Questions
transform
parameter. Should it default toNone
to keep the current behaviour?I think that currently the-> out of scopebbox
of the feature would not be transformed because theextent
is directly accessed. Maybe we need to move the functionality intoget_attribute
.django-rest-framework-gis/rest_framework_gis/serializers.py
Lines 147 to 154 in da5acef