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

handle conversion of label columns #2

Merged
merged 1 commit into from
Sep 20, 2016

Conversation

sjhewitt
Copy link
Contributor

when using sqlalchemy expressions as columns, the selectable is a Label which may be converted, however the Label type may not have doc or nullable attributes. This change uses getattr to try to retrieve them.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage remained the same at 90.83% when pulling 0077f6a on sjhewitt:convert-label-column into c69017e on graphql-python:master.

@syrusakbary syrusakbary merged commit 2bced89 into graphql-python:master Sep 20, 2016
@gsvitak
Copy link

gsvitak commented Sep 25, 2016

I am trying to use a column_property with that maps to an Int. Based on the above change, I think the Int convert should be something like the following.

I have tested locally and it appears to work. Can you please let me know if I am on the right path? I can submit a pull request if think this is the right approach.

Thanks,
Greg

@convert_sqlalchemy_type.register(types.Integer)
def convert_column_to_int_or_id(type, column, registry=None):
    if column.primary_key:
        return ID(description=column.doc, required=not(column.nullable))
    else:
        return Int(description=getattr(column, 'doc', None),
                  required=not(getattr(column, 'nullable', True)))

@sjhewitt
Copy link
Contributor Author

that looks about right - I think we should probably use getattr in each of the converters

@gsvitak
Copy link

gsvitak commented Sep 25, 2016

Great!! I already submitted a pull request for Int. Let me know if I can help with anything else.

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