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
[API change] Getting rid of the create method for observers v0.99999 #601
[API change] Getting rid of the create method for observers v0.99999 #601
Conversation
Co-Authored-By: Rüdiger Busche <ruedigerbusche@web.de>
sacred/observers/sql.py
Outdated
@classmethod | ||
def create_from(cls, engine, session, priority=DEFAULT_SQL_PRIORITY): | ||
"""Second constructor, Java style""" | ||
self = cls.__new__(cls) |
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.
Maybe adding a comment: e.g.: self = cls.__new__(cls) # skip __init__ call
Sorry for taking the fun out of the docstring :D I feel we finally converged on something here! Thanks for hanging on this issue. |
My pleasure. Sacred is an amazing tool. My feeling was that the API wasn't that pretty, this is a good step toward making it very easy to understand and use. I guess a intuitive API is what was missing for a broader adoption. |
Let's get a final review from @Qwlouse , afterwards, we can merge and I'll start working on the API change for the other observers. |
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!
I quite enjoyed our discussion, and the am very happy with the end-result. Thanks everyone for chiming in!
Thanks a lot @boeddeker for your idea! |
No description provided.