Add support for GeometryCollection fields #138
Conversation
There was a problem hiding this comment.
Hi @GJdan, thank you for contributing!
Could you add a test that will help us ensure future mantainers won't accidentally break the GeometryCollection compatibility?
You can add a (short and concise) test here:
https://github.com/djangonauts/django-rest-framework-gis/blob/master/tests/django_restframework_gis_tests/tests.py
Instructions to run tests are here:
https://github.com/djangonauts/django-rest-framework-gis#using-tox
rest_framework_gis/fields.py
Outdated
| args = (geojson,) | ||
| except ValueError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
could you remove this blank line please?
rest_framework_gis/fields.py
Outdated
| @@ -67,6 +62,18 @@ class GeoJsonDict(OrderedDict): | |||
| Used for serializing GIS values to GeoJSON values | |||
| TODO: remove this when support for python 2.6/2.7 will be dropped | |||
| """ | |||
| def __init__(self, *args, **kwargs): | |||
| """ | |||
| If a string is passed attempt to pass it through json.loads, because it should be a geojson formatted string. | |||
There was a problem hiding this comment.
Could you please break the comment like this?
If a string is passed attempt to pass it through json.loads,
because it should be a geojson formatted string.
|
Sure. I have to go to bed now, but I'll push the formatting fixes, and work on a test tomorrow. |
|
@nemesisdesign I've added a test. First, I apologize, I'm not yet as familiar with the testing system as I would like to be. I ripped off the other geojson format tests above to keep some consistency, and also added assertJSONEqual because that's what made sense to me in this kind of test. |
gregseb
changed the title
| else: | ||
| self.assertItemsEqual(json.dumps(response.data), json.dumps(expected)) | ||
| self.assertContains(response, "Kool geometry collection geojson test") | ||
| self.assertJSONEqual(json.dumps(response.data), expected) |
There was a problem hiding this comment.
isn't this line redundant with lines 635-639? If yes I think you can keep this line and safely remove lines 635-639
There was a problem hiding this comment.
Sorry, this was a misunderstanding on my part thinking that assertCountEqual and assertItemsEqual wouldn't do a thorough job because of their names, but on more research I see that they work very well. Line 640 is there because it came with the tests I copied and pasted from test_geojson_format. I have no problem removing it as well.
Removed line 641.
|
Done. I think. Unless you want me to remove the |
|
@GJdan no it's fine, I would just like to ask you one last thing: to squash the commits into one and use as commit message the following: |
gregseb
force-pushed
the
fix-geometrycollection-geojson
branch
from
d01f755 to
5236d78
Compare
|
@nemesisdesign Done |
Fixes #126
We have a project that has a need to support GeometryCollection fields. After poking around I decided to try altering the GeometryField and GeometrySerializerMethodField to use GeoDjango's built-in geojson output instead of creating a GeoJSON object on the fly. It seems to be a small change and fixes the issue in our project without breaking elsewhere.