Allow SQLAlchemy class mapping errors to propagate #281
Conversation
| "You need to pass a valid SQLAlchemy Model in " '{}.Meta, received "{}".' | ||
| ).format(cls.__name__, model) | ||
|
|
||
| if not is_mapped_class(model): |
There was a problem hiding this comment.
assert is only evaluated when the interpreter is running in "debug" mode and is ignored when optimizations are enabled with the -O command line switch. I personally think it's proper to have this check every time and what you have written is an improvement, but to match the previous behavior it might be best to wrap this in an if __debug__: check.
https://docs.python.org/3/reference/simple_stmts.html#grammar-token-assert-stmt
Perhaps
if __debug__ and not is_mapped_class(model):
raise ValueError(...)
| except (ArgumentError, UnmappedClassError): | ||
| except ArgumentError as error: | ||
| # Only handle ArgumentErrors for non-class objects | ||
| if "Class object expected" in str(error): |
There was a problem hiding this comment.
To me, this is a bit of a code-smell. Is it possible to instead create a new exception class and catch that instead? I'm not sure what the import tree looks like, so I'm unsure where it should go, but perhaps:
class InvalidSQLAlchemyModel(ValueError):
pass
...
raise InvalidSQLAlchemyModel("You need to pass a valid SQLAlchemy Model in ... ")
Then there's no need for checking the string of the error message here.
There was a problem hiding this comment.
I agree but I think that would need to be handled in SQLAlchemy itself
|
I really want to see this go in - I'm currently troubleshooting an error hidden by that overly broad except clause. Oh, and just so we're clear, I'm not the maintainer. My suggestions have no weight. |
Good feedback! I'm going to leave things as they are in hopes that @Nabellaleen who requested these changes on #169 approves this should they take a look. |
|
Ping @jnak @Nabellaleen |
|
Closing this in favor of the original PR #169. Thanks for the effort! I agree with @MPickfield that this should be handled upstream in SQLAlchemy. However, @ianepperson if you have more specifics on that error you were tracking, feel free to post them here or open another PR. For now, this is an important change, further improvements are always welcome. |
There's a few PR's open for this already but it seems like they're dead. This PR is the same as #169 with the suggested readability changes.