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

Basic Proxy model support #373

Merged
merged 3 commits into from
Feb 11, 2018

Conversation

jm2242
Copy link
Contributor

@jm2242 jm2242 commented Jan 15, 2018

This PR, in reference to #319 :

  • adds support for querying a table that has proxy model representations in Django, as long as the type is declared to be the superclass (and not the proxy model class). While you could still query for all objects of a particular type, if those objects were cast to a proxy model at runtime, the is_type_of() for the object would fail.
    - adds on_delete where it was breaking tests due to Django 2 requirement
  • fixes tests associated with the Reporter model, to accommodate an additional field reporter_type

In the future, a more intentional way of dealing with Proxy models can be considered.

There are 4 failing tests that are unrelated to this PR

@jm2242 jm2242 changed the title Proxy model support Basic Proxy model support Jan 15, 2018
@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage remained the same at 93.142% when pulling 3c1f67f on jm2242:proxy-model-support into b54e02c on graphql-python:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.142% when pulling 3c1f67f on jm2242:proxy-model-support into b54e02c on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.142% when pulling 3c1f67f on jm2242:proxy-model-support into b54e02c on graphql-python:master.

@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage remained the same at 94.203% when pulling bfcfccf on jm2242:proxy-model-support into e827b10 on graphql-python:master.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Looks good to me, I just commented two things about code style.

Since I'm not familiar with proxy models, maybe it would be worth having a review from @mjtamlyn and @spockNinja if they have some time :)

EDIT: if you rebase from master the failing on the CI should disappear :)

'Reporter Type',
null=True,
blank=True,
choices=[(1, u'Regular'), (2, u'CNN Reporter')])
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you add a line break before the closing )? just to be consistent with the rest of the changes

proxy model support
"""
class Meta:
proxy=True
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you add a space before and after the =?

added support for querying a model with objects that may be a proxy model, fixed and added tests

a few style changes
@jm2242 jm2242 force-pushed the proxy-model-support branch from f70f12d to 0b10341 Compare February 3, 2018 16:12
@jm2242
Copy link
Contributor Author

jm2242 commented Feb 3, 2018

@patrick91 rebased and made the style changes (hopefully I understood the first one)

@patrick91
Copy link
Member

@jm2242 I mean a line break, like this:

...
    choices=[(1, u'Regular'), (2, u'CNN Reporter')]
)

@jm2242
Copy link
Contributor Author

jm2242 commented Feb 3, 2018

that was silly, fixed

Copy link
Contributor

@spockNinja spockNinja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The changes for proxy model support look great.

There is a separate PR dealing with Django 2.0 test support (#336), so if you would slim this PR down to just the proxy model changes and tests, that would be great, and we'll move this PR forward.

Thanks again!

@jm2242
Copy link
Contributor Author

jm2242 commented Feb 4, 2018

no problem @spockNinja, done!

@mjtamlyn
Copy link

mjtamlyn commented Feb 5, 2018

To be honest I know next to nothing about proxy models so probably can't help much here :)

@syrusakbary
Copy link
Member

The PR looks good, merging :)
Thanks for the reviews @spockNinja @patrick91 @mjtamlyn

@syrusakbary syrusakbary merged commit c0edb0c into graphql-python:master Feb 11, 2018
@serates
Copy link

serates commented May 17, 2018

Is graphene-django looking to release a new version and release this to pypi, the last build released was in october. I would like to have access to this fix so that i no longer have to use chain and lists for some of my queries that need proxy support

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.

7 participants