Closed dhaval-mehta closed 3 years ago
OK
Hi, @auvipy Whenever you are free time, please review code quality. Now, I will work on Testcases and Documentation.
I do not know how to generate schema for models.GeometryField
& models.GeometryCollectionField
. Please provide guidance for the schema generation for these fields.
@sir-sigurd would you mind sharing your insight on this? I saw you contribute a lot on geodjango front :)
Hi @auvipy,
I have handled the majority use cases. You can start reviewing code!
great work so far. allow me some time
Another general comment - the current approach doesnt declare components:
and use $ref
, which means that the schema are being duplicated throughout the document if the same GIS field are used multiple times in the models. It would be really nice if the GEO schemas can be emitted only once and $ref
used. I am not sure if DRF supports that yet.
Another general comment - the current approach doesnt declare
components:
and use$ref
, which means that the schema are being duplicated throughout the document if the same GIS field are used multiple times in the models. It would be really nice if the GEO schemas can be emitted only once and$ref
used. I am not sure if DRF supports that yet.
In DRF 3.11, There are no components. Component generation logic got merged recently in the master branch of DRF.
If you look at the master branch code of DRF, components related functions _get_reference
& get_components
internally calls map_serializer
method.
We have overridden the map_serializer
method. Hence, It will automatically generate components with the future's 3.12 release and it will not generate components with DRF 3.11.
This is the expected behavior.
Another general comment - the current approach doesnt declare
components:
and use$ref
, which means that the schema are being duplicated throughout the document if the same GIS field are used multiple times in the models. It would be really nice if the GEO schemas can be emitted only once and$ref
used. I am not sure if DRF supports that yet.In DRF 3.11, There are no components. Component generation logic got merged recently in the master branch of DRF.
If you look at the master branch code of DRF, components related functions
_get_reference
&get_components
internally callsmap_serializer
method.We have overridden the
map_serializer
method. Hence, It will automatically generate components with the future's 3.12 release and it will not generate components with DRF 3.11.This is the expected behavior.
so make this DRF 3.12 only :) no shims needed
Another general comment - the current approach doesnt declare
components:
and use$ref
, which means that the schema are being duplicated throughout the document if the same GIS field are used multiple times in the models. It would be really nice if the GEO schemas can be emitted only once and$ref
used. I am not sure if DRF supports that yet.In DRF 3.11, There are no components. Component generation logic got merged recently in the master branch of DRF. If you look at the master branch code of DRF, components related functions
_get_reference
&get_components
internally callsmap_serializer
method. We have overridden themap_serializer
method. Hence, It will automatically generate components with the future's 3.12 release and it will not generate components with DRF 3.11. This is the expected behavior.so make this DRF 3.12 only :) no shims needed
Alright, I will make this merge request to support only DRF 3.12.
@auvipy DRF 3.12 is out. Should I start work on this again?
@dhaval-mehta go ahead! :+1:
@auvipy DRF 3.12 is out. Should I start work on this again?
i won't mind to make drf-gis as drf 3.12+ only as it improved openAPI support to a great extent
@auvipy DRF 3.12 is out. Should I start work on this again?
i won't mind to make drf-gis as drf 3.12+ only as it improved openAPI support to a great extent
@auvipy improved openAPI support is not compelling enough to drop support to older DRF versions. We should simply make sure the openAPI features throw some kind of error or warning if DRF<3.12 is used, but the rest of the code should keep working with older DRF versions that are listed in the compatibility table.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@nemesisdesign Please check the last run. All openwisp-qa-check
errors are false positive.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
dhaval-mehta, try running: openwisp-qa-format
, I've taken the chance to update the README about this: https://github.com/openwisp/django-rest-framework-gis#running-qa-checks
@nemesisdesign In first screenshot openwisp-qa-check
suggests removing empty space after the comma.
If I do so, flake8 gives me suggestions to add empty space after the comma. The second screenshot describes the suggestions given by flake8.
As per my initial impressions, this is happening only for dictionaries defined in a single line.
@dhaval-mehta in these cases a manual correction is needed. Will try to help asap. I should be able to edit the file from the github interface.
What about the rest of the implementation? Anyone has any feedback for @dhaval-mehta?
@dhaval-mehta in these cases a manual correction is needed. Will try to help asap. I should be able to edit the file from the github interface.
What about the rest of the implementation? Anyone has any feedback for @dhaval-mehta?
It is a bug at the black end. There is also one solution. Let me try. Ref: https://github.com/psf/black/issues/1289
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Something went wrong with the build.
TravisCI finished with status errored
, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.
Hey @dhaval-mehta, Something went wrong with the build.
TravisCI finished with status errored
, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.
Hey @dhaval-mehta, Something went wrong with the build.
TravisCI finished with status errored
, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.
Hey @dhaval-mehta, Something went wrong with the build.
TravisCI finished with status errored
, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Something went wrong with the build.
TravisCI finished with status errored
, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey @dhaval-mehta, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@nemesisdesign @auvipy Code is ready for the final review. 🔥 This PR does not implement schema generation for geometry collection and geometry field. I will raise separate PR soon(may take up to 2 months).
This PR does not implement schema generation for geometry collection and geometry field. I will raise separate PR soon(may take up to 2 months).
If it isnt going to be done in this PR, please create an issue about it after this PR is merge, as this should hold up a release, or at least be relnoted prominently, otherwise users will data using those types will have a broken schema, and how this might effect their systems feels like it could be unpredictable.
@auvipy done
why?
These commits are no longer required. #248 fixes PostgreSQL integration problem.
@auvipy Gentle reminder. I have merged the latest master.
I guess the only way to get feedback on this feature is to publish it and see what happens, @dhaval-mehta will you stick around to help us maintain it?
Sure. It will be my pleasure to maintain this project.
@dhaval-mehta your work is now released! I expect feedback from users of this library in the coming months. I sent you an invite to have write access to this repository in the hope that you can help us maintain this feature as feedback from users comes in.
Pushes to master are restricted to admins, but pushes to other branches are allowed :+1:.
I will ask my network to test this as well. It will help us to improve the schema generation quickly.
@dhaval-mehta your work is now released! I expect feedback from users of this library in the coming months. I sent you an invite to have write access to this repository in the hope that you can help us maintain this feature as feedback from users comes in.
Pushes to master are restricted to admins, but pushes to other branches are allowed 👍.
Thank you so much for providing me this opportunity. It would be my pleasure to contribute to this repo.
Requiring users to set REST_FRAMEWORK.DEFAULT_SCHEMA_CLASS to a custom GIS specific class will limit the utility of this. The default DRF schema generator is rarely used for serious projects because there are so many unsolved problems in it. Most medium to large projects will use drf-yasg, or similar (including drf-spectacular which is new but quite good), and they use their own mechanism, either requiring that DEFAULT_SCHEMA_CLASS is set to their own class, or they ignore DEFAULT_SCHEMA_CLASS altogether. And many projects need to sub-class DRF's AutoSchema to add their own behaviour. I've tried to do a WIP for drf-spectacular to use the standard AutoSchema inheritance at tfranzel/drf-spectacular#46 , but it has has some curly problems.
Keeping GeoFeatureAutoSchema is ok, but it would be better as a thin wrapper only, using functionality that exists outside of the class.
i.e. the schemas and functions that are embedded here inside this class should be in the module namespace, so they can be used with other generators. The code in this custom AutoSchema only uses
self.
to reference the new code below -- it doesnt rely on any of the inner workings of DRF AutoSchema.
@dhaval-mehta Is there any way now that the schema generation is compatible with other schema generators, such as drf-spectacular? If not, is there a way we can make it compatible?
Requiring users to set REST_FRAMEWORK.DEFAULT_SCHEMA_CLASS to a custom GIS specific class will limit the utility of this. The default DRF schema generator is rarely used for serious projects because there are so many unsolved problems in it. Most medium to large projects will use drf-yasg, or similar (including drf-spectacular which is new but quite good), and they use their own mechanism, either requiring that DEFAULT_SCHEMA_CLASS is set to their own class, or they ignore DEFAULT_SCHEMA_CLASS altogether. And many projects need to sub-class DRF's AutoSchema to add their own behaviour. I've tried to do a WIP for drf-spectacular to use the standard AutoSchema inheritance at tfranzel/drf-spectacular#46 , but it has has some curly problems. Keeping GeoFeatureAutoSchema is ok, but it would be better as a thin wrapper only, using functionality that exists outside of the class. i.e. the schemas and functions that are embedded here inside this class should be in the module namespace, so they can be used with other generators. The code in this custom AutoSchema only uses
self.
to reference the new code below -- it doesnt rely on any of the inner workings of DRF AutoSchema.@dhaval-mehta Is there any way now that the schema generation is compatible with other schema generators, such as drf-spectacular? If not, is there a way we can make it compatible?
This implementation is compatible with all libraries which are compatible with DRF schema generation. As per my knowledge, implementation of drf-spectacular is not compatible with DRF's schema generation. Hence I think, there is no way to make it compatible with drf-spectacular.
https://github.com/tfranzel/drf-spectacular/pull/38 This PR is open. This can bring support for open API schema generation for drf-spectacular.
Hi @auvipy,
This fixes: #219, #237
You requested to wait for 3.12 Release. I will write code in such a way that it will be compatible will 3.12 Release.
This will close: #186 and #219