-
Notifications
You must be signed in to change notification settings - Fork 79
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
Standardize model __repr__
functions
#1140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but there are a few more inconsistencies that might be good to address I think!
Thanks for the eye, @AetherUnbound! I'll address those tomorrow and get back to you when they're done. |
I don't know enough about Python to feel super comfortable reviewing it so I'll let the rest of the reviewers weigh in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the inconsistencies Madison pointed out, this looks good! If we're being pedantic, the Python spec technically says you should be able to instantiate the object by eval-ing the string returned by __repr__
https://docs.python.org/3/library/functions.html#repr
I also wonder if we could get these for free by just making our model classes dataclasses: https://docs.sqlalchemy.org/en/20/orm/dataclasses.html
These are all good comments, thank you! Please excuse the delay in my response, I've been holday'ing. I'll take a look at the |
Looking at the changes needed to implement the
|
Description of Changes
Addressed inconsistencies with the model
__repr__
functions.Mostly making this because this PR got a bit out of control: #1138
Tests and Linting
develop
branch.pytest
passes on my local development environment.pre-commit
passes on my local development environment.