-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
DataclassJSONField
implementation
#77
base: master
Are you sure you want to change the base?
Conversation
@@ -24,6 +24,8 @@ class DjangoArrayField: | |||
|
|||
|
|||
class JSONField(DjangoJSONField): | |||
form_class = JSONFormField |
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.
so it's overridable
self, | ||
dataclass_cls: Type[DataclassJsonSchema], | ||
*, | ||
many: bool = False, |
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 many
convention comes from Django Rest Framework
But we can consider making this a DataclassArrayJSONField
instead
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'm going to leave this decision up to you. I'm not familiar with the dataclasses-jsonschema
library. So, whatever you think works best, we can go with that.
@@ -0,0 +1,3 @@ | |||
# `django_jsonform.contrib.dataclasses` | |||
|
|||
**Status:** experimental |
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.
TODO
import dataclasses | ||
from typing import Type | ||
|
||
from dataclasses_jsonschema import JsonSchemaMixin |
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 don't think we should add this as a dependency to this project.
Need to research how to make this contrib
module optional.
Maybe removing the contrib/__init__,py
would work?
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 is okay.
To make it an optional dependency, please modify the setup.cfg
file and add this at the end:
+
+ [options.extras_require]
+ contrib-dataclasses = dataclasses-jsonschema
Users who wish to use this feature can install the dependencies by:
pip install "django-jsonform[contrib-dataclasses]"
@bhch , I'm unsure if this should become it's own package |
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'm unsure if this should become it's own package
django-dataclass-field
. Thoughts?
Maybe.
If the goal is to have a dataclass interface to work with json data, why can't we write a function (or a method on JSONField
) to convert the json data to a dataclass?
def json_to_dataclass(json_data, dataclass):
return dataclass_instance
We can also accept a dataclass for the schema
argument. And then do the conversions internally.
Anyway, I've reviewed the changes and left some comments.
import dataclasses | ||
from typing import Type | ||
|
||
from dataclasses_jsonschema import JsonSchemaMixin |
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 is okay.
To make it an optional dependency, please modify the setup.cfg
file and add this at the end:
+
+ [options.extras_require]
+ contrib-dataclasses = dataclasses-jsonschema
Users who wish to use this feature can install the dependencies by:
pip install "django-jsonform[contrib-dataclasses]"
|
||
schema["type"] = "array" | ||
schema["description"] = f"An array of {dataclass_cls.__name__} objects." | ||
del schema["required"] |
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.
The code example in your comment above gives me KeyError: 'required'
. Perhaps use this instead:
del schema["required"] | |
schema.pop("required", None) |
schema["type"] = "array" | ||
schema["description"] = f"An array of {dataclass_cls.__name__} objects." | ||
del schema["required"] | ||
schema["items"] = schema["properties"]["item"] |
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.
Line 21 also results in a KeyError: 'item'
.
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.
Hmm.. This shouldn't be happening.
What Python and dataclasses-jsonschema versions are you using?
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.
Python: 3.8
dataclasses-jsonschema: 2.16.0
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'm using dataclasses-jsonschema[fast-validation]==2.16.0
and Python 3.10
Will have to try 3.8 later (or make a separate package and drop support for older versions)
I noticed you're supporting a lot of older Django versions too. I only tested with Django==4.1.3
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.
Just tested with Python 3.10 and Django 4.1.3. Everything works fine this time.
self, | ||
dataclass_cls: Type[DataclassJsonSchema], | ||
*, | ||
many: bool = False, |
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'm going to leave this decision up to you. I'm not familiar with the dataclasses-jsonschema
library. So, whatever you think works best, we can go with that.
FYI: I am @ghost. I had to delete my other account for logistics reasons. I'm starting to lean towards making a separate package because I don't have time to investigate and support older Python/Django versions. My next step is to fix the "required" fields, because on Admin, I can't submit empty for fields like |
@bhch does your library support json schema with Edit: Looks like No. It fails here |
Currently,
That's understandable. I'd like to avoid breaking changes until the next major release (v3). |
Another question @bhch , I noticed that the Django admin form sends Empty String for empty integer field, rather than null I know this is probably because it's hard to tell the difference between null and no input and empty string. One thing Swagger UI does is they have a tick box [] Send empty value to indicate sending a null. Thoughts on adding this to the Form? |
@dongyuzheng Okay, I'll add something similar to that. I've just created an issue for this: #78 |
as discussed in #75
WIP WIP WIP
Example