Skip to content

Implement Mechanism to Selectively Override Automatic Field Creations #214

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

Merged
merged 5 commits into from
Jun 7, 2019

Conversation

jnak
Copy link
Collaborator

@jnak jnak commented May 14, 2019

This implements the overriding mechanism discussed in #209.

Changes include:

  • Add ORMField class
  • The main overridable parameters are: type, description, deprecation_reason and required
  • We can name fields differently using prop_name. This was preferred over name to avoid confusion / collision with graphene.Field parameters.
  • Add tests for all types of SQLAlchemy properties: columns, relationships, column properties, hybrid properties and and composite properties.
  • Cleanups and re-organize some tests.

TODOs that I'm planning to address as soon as this PR is merged:

  • Add a way to override relationship types.
  • Add a way to override composite fields default parameters.
  • Add example and documentation

@jnak jnak requested review from Cito and Nabellaleen May 14, 2019 20:46
@coveralls
Copy link

coveralls commented May 14, 2019

Coverage Status

Coverage increased (+3.1%) to 97.093% when pulling 36675c0 on jnak:saot-override into 8ad1f75 on graphql-python:master.

@@ -1,50 +0,0 @@
from py.test import raises
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI I moved those tests to test_types.py because it was not clear where to add new tests.

Copy link
Collaborator Author

@jnak jnak 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 feedback @wyattanderson!

wyattanderson
wyattanderson previously approved these changes Jun 3, 2019
Copy link

@wyattanderson wyattanderson left a comment

Choose a reason for hiding this comment

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

LGTM

Nabellaleen
Nabellaleen previously approved these changes Jun 6, 2019
Copy link
Collaborator

@Nabellaleen Nabellaleen left a comment

Choose a reason for hiding this comment

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

Soooooo nice !

Add documentation would be very nice, to see a "how to" automatically integrated to the website published documentation.
Do you plan to do it in another PR ? Otherwise, could you add it in this one ?

query = """
query ReporterQuery {
query {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to "de-name" the query here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was no reason to name the query so I removed to make tests more consistent and simple.

@@ -200,42 +330,34 @@ class _TestSQLAlchemyConnectionField(SQLAlchemyConnectionField):


def test_default_connection_field_factory():
_registry = Registry()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ? ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that was not needed because the global registry is reset between tests (see confest.py).

Copy link
Collaborator Author

@jnak jnak left a comment

Choose a reason for hiding this comment

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

@Nabellaleen Thanks for the review!

For now, I've added detailed docstrings witth examples to ORMField. I'm planning to add proper documentation in an upcoming PR once I'm done relationships and hybrid overriding (see my first comment on this thread).

Cheers

query = """
query ReporterQuery {
query {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was no reason to name the query so I removed to make tests more consistent and simple.

@@ -200,42 +330,34 @@ class _TestSQLAlchemyConnectionField(SQLAlchemyConnectionField):


def test_default_connection_field_factory():
_registry = Registry()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that was not needed because the global registry is reset between tests (see confest.py).

@jnak jnak merged commit 9a0f740 into graphql-python:master Jun 7, 2019
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.

4 participants