WIP: Add drf-gis fields #38

jayvdb · 2020-04-25T04:18:39Z

Closes #31

tests/test_extensions.py

tfranzel

@jayvdb thank you very much for contributing this. have a look at the comments. lets try to reevaluate after those changes but it looks already very promising i think.

tests/test_extensions.py

tfranzel · 2020-04-25T10:39:00Z

tests/test_extensions.py

+    assert schema['components']['schemas']['X']['properties']['hash']['format'] == 'byte'
+
+
+def test_serializer_field_extension_geo(no_warnings):


please move contib tests to tests/contib/PACKAGE_NAME

also would like to see a full schema test for that. have a look at test_oauth_toolkit for reference. makes it easier to inspect that the generated schema look reasonable.

tfranzel · 2020-04-25T10:40:12Z

drf_spectacular/contrib/fields.py

+    target_class = 'rest_framework_gis.fields.GeometryField'
+
+    def map_serializer_field(self, auto_schema):
+        return {


use component as discussed here: #31 (comment)

tfranzel · 2020-04-25T10:40:26Z

drf_spectacular/contrib/fields.py

+
+    def map_serializer_field(self, auto_schema):
+        return {
+            'oneOf': [


same here i guess

jayvdb · 2020-05-02T14:33:12Z

drf_spectacular/contrib/fields.py

+        'type': {
+            'description': 'The type of the geos instance',
+            'type': 'string',
+            'enum': ['Feature'],


I think this should be 'FeatureCollection'

codecov · 2020-05-03T04:31:27Z

Codecov Report

Merging #38 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   96.79%   96.85%   +0.05%     
==========================================
  Files          38       39       +1     
  Lines        2496     2541      +45     
==========================================
+ Hits         2416     2461      +45     
  Misses         80       80

Impacted Files	Coverage Δ
drf_spectacular/contrib/fields.py	`100.00% <100.00%> (ø)`
drf_spectacular/openapi.py	`92.50% <100.00%> (+0.01%)`	⬆️
tests/test_extensions.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 749dfb6...2ba7de7. Read the comment docs.

tfranzel · 2020-05-08T13:07:10Z

hey @jayvdb i can't really tell the difference between the last force pushes. what changed? could you do the remaining change requests so we can wrap this up? thanks

Xenial results in libgdal.so: undefined symbol: OGR_F_GetFieldAsInteger64 Related to tfranzel#31

jayvdb · 2020-06-06T10:13:02Z

The pushes are me rebasing my WIP to stay on top of changes to master.
We've had some more testing occur on our side. Still a bit more to do re correctness; then the technical fixes needed.

jayvdb · 2020-07-21T12:08:08Z

This is blocked by #130 , as it uses anyOf.

jayvdb · 2020-10-15T13:39:03Z

Note that openwisp/django-rest-framework-gis#223 is now progressing, and provides the appropriate basis for this PR.

Please close #67 and let me finish my own PR, rather than merging incomplete PRs like you did with #125. That is basic respect for people contributing to your project.

tfranzel · 2020-10-15T14:25:42Z

that is good to hear! as a matter of fact i have updated (but not yet pushed) my adaptation in #67 a few days ago. I will happily close it if your/their new approach is better, which i believe it will be. in the meantime i will not continue working on #67

regarding #125: i took the liberty of completing your PR because it was stale for months with pending change requests. you indicated that you were too out-of-sync, lacking time, and basically moved on. i decided it was better for the project to finish it myself instead of not having it at all. imho waiting 3 months for a follow-up is hardly disrespecting your contribution.

jayvdb · 2020-10-16T01:06:50Z

Out of sync is regarding ability to test your patches against my real systems. It has nothing to do with my ability to complete PRs.

regarding #125: it was stale because it was waiting for you to finish a solution to #101 - you already cookie licked that one. You never gave a reply to #125 (comment) , which was the last exchange there. So who was "waiting 3 months for a follow-up" ?

tfranzel · 2020-10-16T07:46:41Z

fair enough. i'm sorry that the open question fell off my radar. i invested more time in doing a detailed review of #125 (as requested) than actually completing the PR, which seems like quite an inefficient cookie licking tactic. you are of course entitled to your opinion, but i will no longer respond to accusational or passive aggressive comments.

nemesifier · 2021-01-25T00:24:46Z

openwisp/django-rest-framework-gis#223 merged and released.

AndrewGuenther · 2021-07-10T23:52:41Z

What else needs to happen to get this landed? I'd really like to see support for this.

jayvdb · 2021-07-11T00:30:04Z

I think there were a few things in that PR which were deferred to later, and I havent been tracking this closely.
c.f. openwisp/django-rest-framework-gis#223 (comment)

I'll get this rebased next week, and work out the current status of bits outstanding.

tfranzel · 2021-07-11T08:47:39Z

it looks to me that they went with the official recommendation of patching AutoSchema, which is in conflict with our AutoSchema. @jayvdb made the very good point there that this solution simply does not scale well. I frankly do not understand how this recommendation was ever supposed to work for non-trivial cases. If someone is to use 2 different libs (e.g. drf-gis and something else) that each patch AutoSchema, you are already in a deadlock, because they cannot be easily reconciliated.

The core question is whether we can extract their spec from their AutoSchema in a reasonable way without bending backwards too much. @jayvdb suggested a thin wrapper on their side to make their implementation also usable without the AutoSchema. I do not currently know if that was considered or not. It would make things a lot easier.

if we were able to use their implementation, the code in this PR is pretty much obsolete. The likely outcome would be to use Serializer(Field)Extension that extract schema info from their AutoSchema/codebase. @AndrewGuenther i hope that clears things up.

@jayvdb thanks for picking this up again.

jayvdb · 2021-07-11T09:07:12Z

I'm going to take a quick look this week, to get a plan in place, but I'll be more focused on Django in a few weeks when I can devote some serious time to it, and likely get some patches into drf-gis building on top of what they have done.

AndrewGuenther · 2021-07-13T17:27:53Z

@tfranzel Yeah, that recommendation is quite silly. I had tried to make a change where drf-gis would inherit from DefaultSchema instead, but that leads to circular dependency problems if their generator is the default. There's really just not a great way to go about it that I can see. Maybe if DefaultSchema was smart enough to check that the current class is the default and just turns itself into a no-op? That would take a change both in drf as well as drf-gis to make work, but then wouldn't require any effort here.

auvipy

would be really a great addition! can the review comments be adressed?

tfranzel · 2022-05-22T13:50:42Z

this PR got superseded by #746. discussion&feedback continues there.

jayvdb mentioned this pull request Apr 25, 2020

django-rest-framework-gis #31

Closed

jayvdb commented Apr 25, 2020

View reviewed changes

tests/test_extensions.py Show resolved Hide resolved

tfranzel requested changes Apr 25, 2020

View reviewed changes

jayvdb mentioned this pull request Apr 28, 2020

Add support for OpenAPI Schema Generation openwisp/django-rest-framework-gis#219

Closed

jayvdb commented May 2, 2020

View reviewed changes

jayvdb mentioned this pull request May 2, 2020

WIP: AutoSchema: Inherit DRF AutoSchema #46

Closed

jayvdb force-pushed the geo branch 4 times, most recently from 323020f to 3a7823c Compare May 3, 2020 03:47

jayvdb force-pushed the geo branch 2 times, most recently from 1f611b3 to 2a42819 Compare May 7, 2020 22:35

jayvdb added 2 commits May 22, 2020 11:10

Add drf-gis fields

b75492c

.travis.yml: Add gdal on bionic

2ba7de7

Xenial results in libgdal.so: undefined symbol: OGR_F_GetFieldAsInteger64 Related to tfranzel#31

jayvdb force-pushed the geo branch from 2a42819 to 2ba7de7 Compare May 22, 2020 04:13

tfranzel mentioned this pull request May 24, 2020

GIS fixes for #38 #67

Closed

jayvdb mentioned this pull request Oct 18, 2020

Invalid openAPI emitted when nullable and no top level type #130

Closed

tfranzel mentioned this pull request Sep 23, 2021

Using restframework-gis OpenAPI schema creation #532

Closed

dhaval-mehta mentioned this pull request Sep 24, 2021

OpenAPI Schema Generation openwisp/django-rest-framework-gis#223

Merged

auvipy suggested changes Oct 14, 2021

View reviewed changes

joey-kevala mentioned this pull request May 18, 2022

Add rest_framework_gis blueprint #743

Closed

This was referenced May 19, 2022

add django-rest-framework-gis upstream approach #744

Closed

django-rest-framework-gis support #746

Merged

tfranzel closed this May 22, 2022

keithamoss mentioned this pull request May 23, 2022

Add drf-gis fields to the API doco keithamoss/demsausage#1361

Closed

keithamoss mentioned this pull request Jan 5, 2023

Supporting API integration by third parties keithamoss/demsausage#49

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add drf-gis fields #38

WIP: Add drf-gis fields #38

jayvdb commented Apr 25, 2020

tfranzel left a comment

tfranzel Apr 25, 2020

tfranzel Apr 25, 2020

tfranzel Apr 25, 2020

jayvdb May 2, 2020

codecov bot commented May 3, 2020 •

edited

Loading

tfranzel commented May 8, 2020

jayvdb commented Jun 6, 2020

jayvdb commented Jul 21, 2020

jayvdb commented Oct 15, 2020

tfranzel commented Oct 15, 2020

jayvdb commented Oct 16, 2020

tfranzel commented Oct 16, 2020

nemesifier commented Jan 25, 2021

AndrewGuenther commented Jul 10, 2021

jayvdb commented Jul 11, 2021

tfranzel commented Jul 11, 2021

jayvdb commented Jul 11, 2021

AndrewGuenther commented Jul 13, 2021

auvipy left a comment

tfranzel commented May 22, 2022

		assert schema['components']['schemas']['X']['properties']['hash']['format'] == 'byte'


		def test_serializer_field_extension_geo(no_warnings):

WIP: Add drf-gis fields #38

WIP: Add drf-gis fields #38

Conversation

jayvdb commented Apr 25, 2020

tfranzel left a comment

Choose a reason for hiding this comment

tfranzel Apr 25, 2020

Choose a reason for hiding this comment

tfranzel Apr 25, 2020

Choose a reason for hiding this comment

tfranzel Apr 25, 2020

Choose a reason for hiding this comment

jayvdb May 2, 2020

Choose a reason for hiding this comment

codecov bot commented May 3, 2020 • edited Loading

Codecov Report

tfranzel commented May 8, 2020

jayvdb commented Jun 6, 2020

jayvdb commented Jul 21, 2020

jayvdb commented Oct 15, 2020

tfranzel commented Oct 15, 2020

jayvdb commented Oct 16, 2020

tfranzel commented Oct 16, 2020

nemesifier commented Jan 25, 2021

AndrewGuenther commented Jul 10, 2021

jayvdb commented Jul 11, 2021

tfranzel commented Jul 11, 2021

jayvdb commented Jul 11, 2021

AndrewGuenther commented Jul 13, 2021

auvipy left a comment

Choose a reason for hiding this comment

tfranzel commented May 22, 2022

codecov bot commented May 3, 2020 •

edited

Loading