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

Don't suppress SQLAlchemy errors when mapping classes #169

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

connorbrinton
Copy link
Contributor

@connorbrinton connorbrinton commented Oct 12, 2018

Currently, many SQLAlchemy model definition errors are hidden the way we check if valid models have been provided. This PR allows SQLAlchemy model definition errors to propagate at model check time, and adds a test to confirm that errors are correctly propagated.

Fixes #121

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 91.877% when pulling 21a3b89 on connorbrinton:master into 33d5b74 on graphql-python:master.

@coveralls
Copy link

coveralls commented Oct 12, 2018

Coverage Status

Coverage increased (+0.1%) to 91.989% when pulling cad1e49 on connorbrinton:master into 6a96d37 on graphql-python:master.

@connorbrinton connorbrinton changed the title Don't suppress SQLAlchemy errors when mapping classes, fixes #121 Don't suppress SQLAlchemy errors when mapping classes Oct 12, 2018
@indigoviolet
Copy link

is this blocked?

@connorbrinton
Copy link
Contributor Author

Nope, everything should be good to go! I just rebased on master to make sure it's up to date with the latest changes as well.

@lanshark
Copy link

I implemented these changes in a local virtualenv to test an error I was getting. I can confirm that these changes DO indeed fix the problem.

@alefhsousa
Copy link

Hi guys, I got this error today. We have any problem to increase these changes on master?

graphene_sqlalchemy/utils.py Outdated Show resolved Hide resolved
These changes modify graphene-sqlalchemy to not suppress errors coming
from SQLAlchemy when attempting to map classes. Previously this made the
debugging experience difficult, since issues with SQLAlchemy models
would produce an unclear error message from graphene-sqlalchemy. With
these changes, the SQLAlchemy error is propagated to the end-user,
making it possible for them to easily correct the true issue.
@codecov-commenter
Copy link

Codecov Report

Merging #169 (8205df7) into master (869a55b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   96.92%   96.95%   +0.02%     
==========================================
  Files           9        9              
  Lines         618      623       +5     
==========================================
+ Hits          599      604       +5     
  Misses         19       19              
Impacted Files Coverage Δ
graphene_sqlalchemy/types.py 94.35% <100.00%> (+0.04%) ⬆️
graphene_sqlalchemy/utils.py 95.89% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 869a55b...8205df7. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLAlchemyObjectType hides original SQLAlchemy exception
10 participants