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

Fixed unnecessary GDAL dependency. #100

Merged
merged 3 commits into from
Dec 5, 2018
Merged

Conversation

coredumperror
Copy link
Contributor

This PR fixes #75, as far as I can tell. At least, I can get through a manage.py makemigrations now while using GeoJSONLayerView in one of my files. There may be other places where this check for ImproperlyConfigured needs to be added.

Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

The test test_view_trims_to_geometries_boundaries is failing apparently

@@ -149,7 +150,8 @@ def get_queryset(self):
# Won't trim point geometries to a boundary
model_field = qs.model._meta.get_field(self.geometry_field)
self.trim_to_boundary = (self.trim_to_boundary and
not isinstance(model_field, PointField))
not isinstance(model_field, PointField) and
Intersection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Intersection is not None would be easier to read I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fix incoming.

@coredumperror
Copy link
Contributor Author

The failing tests are all for Django 1.8, which has been desupported for more than 6 months. Every other test suite passes.

I could change the patch to not affect the Django 1.8 code path, but then the two code paths would have different behavior based on whether GDAL is installed or not, even though the Django 1.8 code (apparently) doesn't use it.

So I'm not sure what the right solution is.

@leplatrem
Copy link
Collaborator

The devs at makinacorpus should confirm if they're ok with that breaking change (ie. dropping 1.8)

Otherwise, at least drop them from Travis

@leplatrem
Copy link
Collaborator

@gutard @Gagaro @LePetitTim ping?

@gutard
Copy link
Collaborator

gutard commented Dec 4, 2018

The devs at makinacorpus should confirm if they're ok with that breaking change (ie. dropping 1.8)

Geotrek now requires Django 1.11 so it's OK to drop Django 1.8 support.

@Gagaro
Copy link
Member

Gagaro commented Dec 5, 2018

I'm ok with dropping support for 1.8 as well. All the travis test are failing because of travis though, I'll take care of fixing everything and updating the travis conf.

@Gagaro Gagaro merged commit 1f7135b into makinacorpus:master Dec 5, 2018
@Gagaro
Copy link
Member

Gagaro commented Dec 5, 2018

Released in 2.12.0. Thanks @coredumperror 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serializer still depends on GDAL?
4 participants