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

add support for django 3.1 JSONField #85

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

xi
Copy link
Contributor

@xi xi commented Jul 20, 2020

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Hi @xi thanks for your PR!!! The code looks great but can I ask you for 2 improvements:

  1. Can you fix the error raised by CI?

  2. Can you add an unit test to ensure support for Django's JSONField? It should be very similar to the existing one for postgres' JSONField implementation.

If you have any questions, don't hesitate on pinging me for help =)

@xi xi force-pushed the add-cross-db-json-field branch from 8d56ec4 to 401bc99 Compare July 20, 2020 16:25
@xi
Copy link
Contributor Author

xi commented Jul 20, 2020

Can you fix the error raised by CI?

Unfortunately I was not able to figure out what I need to change. The output only says that black would reformat a file, but not what it would change about that file. Running black locally did not result in the message.

Can you add an unit test to ensure support for Django's JSONField?

I pushed a test. I could not figure out how to run the tests locally so I am waiting for the results from CI.

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks @xi for updating your PR!!! It was very useful no only because of the changes you're introducing, but also because it exposed for us a gap in our docs when it comes to code contributions (running tests, link, etc). We've opened #86 so future contributors won't have the same problems you had.

I think there are a few things preventing us from merging your PR:

  1. Wait for Improve contributing instructions and utilitaries #86 to be merged so we can rely on the make lint command or the pre-commit hooks to make sure the code format is right;
  2. Updates to the CHANGELOG.md;
  3. Wait for the official Django 3.1 release to go live. For now it's still under the alpha version. Once we have it released, we'll be able to easily configure our tox.ini by adding the django31 dependency;

Thanks a lot!

@xi xi force-pushed the add-cross-db-json-field branch from 401bc99 to 1825192 Compare July 22, 2020 17:40
@xi
Copy link
Contributor Author

xi commented Jul 22, 2020

Updates to the CHANGELOG.md;

done

Wait for the official Django 3.1 release to go live.

Why wait? The change should be fully backwards compatible. (I am fine if you prefer waiting, I just wanted to make sure there is no misunderstanding)

@xi xi force-pushed the add-cross-db-json-field branch from 1825192 to 7787ecb Compare July 22, 2020 17:47
@xi xi force-pushed the add-cross-db-json-field branch from 7787ecb to 234f90a Compare July 22, 2020 18:04
Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

@xi I actually forgot to mention we were waiting to release the new 1.1.1 version.

Now I'm good with merging this in. @anapaulagomes or @amureki can take a look as well?

Copy link
Contributor

@anapaulagomes anapaulagomes left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Collaborator

@amureki amureki left a comment

Choose a reason for hiding this comment

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

👍

@berinhard berinhard merged commit d696a57 into model-bakers:master Jul 22, 2020
@xi
Copy link
Contributor Author

xi commented Jul 22, 2020

I think I made a mistake when rebasing and the changelog entry is now in the wrong release (1.1.1). I won't be able to fix it until monday so maybe someone else could do it.

amureki pushed a commit that referenced this pull request Jul 23, 2020
@amureki
Copy link
Collaborator

amureki commented Jul 23, 2020

@xi oh, no worries, we missed this somehow. :) Good catch! I put a PR here: #92

berinhard pushed a commit that referenced this pull request Jul 24, 2020
@snicoletbcg
Copy link

Hi,
Thanks for this patch wich is eactly what i needed but i'm not sure this was correctly merged into 1.1.1 since i got the following error:

TypeError: <class 'django.db.models.fields.json.JSONField'> is not supported by baker.

also in baker.generators i've saw that:

try:
    from django.contrib.postgres.fields import JSONField
except ImportError:
    JSONField = None

@xi
Copy link
Contributor Author

xi commented Aug 20, 2020

Hi @snicoletbcg. This patch is not yet included in 1.1.1. You will have to wait for the next release.

In the meantime you could use this workaround:

from django.contrib.postgres.fields import JSONField as PostgresJSONField
from django.db.models import JSONField
from model_bakery import baker

baker.generators.add(
    JSONField, baker.generators.default_mapping[PostgresJSONField]
)

@snicoletbcg
Copy link

Hi @xi ,
thank you for your answer,
i've put

model_bakery = { git = "https://github.com/model-bakers/model_bakery.git", rev = "06c9fd8" }

into my pyproject.toml (poetry) and it worked perfectly (maybe it can help someone else)

and then

poetry update

it worked fine, thank you again

Copy link
Contributor

@israelst israelst left a comment

Choose a reason for hiding this comment

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

I have suggested an import fixing at #106

@@ -89,7 +89,16 @@ class Person(models.Model):
id_document = models.CharField(unique=True, max_length=10)

try:
from django.contrib.postgres.fields import ArrayField, HStoreField, JSONField
from django.models import JSONField
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the correct import is django.db.models, isn't it?

@@ -19,14 +19,19 @@
from model_bakery.random_gen import gen_related
from tests.generic import generators, models

try:
from django.models import JSONField
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the correct import is django.db.models, isn't it?

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.

6 participants