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

docs: Correct schema parsing for JSONField #5878

Merged
merged 2 commits into from Apr 20, 2018
Merged

docs: Correct schema parsing for JSONField #5878

merged 2 commits into from Apr 20, 2018

Conversation

beruic
Copy link
Contributor

@beruic beruic commented Mar 13, 2018

This fixes #5873.

@carltongibson
Copy link
Collaborator

Hi @beruic. Can you add a regression test for this too please? Thanks.

@beruic
Copy link
Contributor Author

beruic commented Mar 13, 2018

Well, I would like to, but this is new ground for me, time is sparse, and i already spend a lot of time tracking this down. I have tested this informally, and now I have looked at how to write a test for this for about an hour, and I cannot figure out how to test this.

Perhaps there should just be made an issue to test rest_framework.schemas.inspectors.field_to_schema() by it self? This of course means that all the other fields should be tested, but then again, it feels kinda stupid to do. It really any value. I think it would be valuable if you could test the docs-generated HTML for each field, but that is still way beyond my scope of understanding of DRF.
Perhaps it would make sens to make it a part of some test of rest_framework.schemas.inspectors.AutoSchema, but again that is beyond my scope.

If it really is the case that none of the other output of rest_framework.schemas.inspectors.field_to_schema()is tested, I think this should just be merged, and an issue should be created to figure out how to test.
All in all I think this is a bigger task, but perhaps you are wiser @carltongibson?

@carltongibson
Copy link
Collaborator

The test coverage here is sub-optimal. All the schema related code is still quite new and so this is to be expected.

As we make changes we want to add coverage. We try not to add changes without a regression test to make sure future changes don't break anything.

A test on field_to_schema() will be fine. I'm happy for you just to test the JSONField case. We can add other cases as a separate clean-up later.

Thanks for the effort!

@beruic
Copy link
Contributor Author

beruic commented Mar 14, 2018

I have added tests now, testing JSONField and the simple cases of rest_framework.schemas.inspectors.field_to_schema(). The test is prepared for adding more cases easily, as long as you know what you are doing ;)

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Yep. This looks good. Thanks @beruic.

@carltongibson carltongibson added this to the 3.8.3 Release milestone Apr 20, 2018
@carltongibson carltongibson merged commit 5ee0e5d into encode:master Apr 20, 2018
@beruic beruic deleted the patch-2 branch April 23, 2018 08:53
@rpkilby rpkilby modified the milestones: 3.8.3 Release, 3.9 Release Aug 29, 2018
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
Fixes encode#5873.
* Use Object type. 
* Add test for field_to_schema
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.

docs: JSON input to JSONField is not parsed correctly
3 participants