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

Let __init__ of your model get access to all given properties #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Jan 11, 2019

In some scenario your model might define some default values based on properties given at creation time.
This is only possible if __init__ receives all properties at once.

In some scenario your model might define some default values
 based on properties given at creation time.
This is only possible if __init__ receives all properties at once.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 89.111% when pulling 7a4996e on ticosax:initialize-model-as-python-expect-it into 296d244 on biosustain:master.

@lyschoening
Copy link
Contributor

Unfortunately, this will not always work as the constructor of a model sometimes has unexpected behavior.

@ticosax
Copy link
Contributor Author

ticosax commented Jan 14, 2019

The current implementation is slower due to iteration of every properties and not very pythonic. If there is an expected way of constructing an instance of a class is by passing all the argument to the constructor. Initializing an instance and then calling setattr for all properties is anti-pattern I would say.
Our project overrides the create method on several locations already because of this.
The current implementation forces projects to defer from the generic behaviour. I don't know what are your constraints, but potion should not impose this particular case to all projects. The adoption of specific behaviour should be done in specific projects, not for everyone.

@lyschoening
Copy link
Contributor

I agree that it is not ideal, but there are legitimate issues with the other approach that led to this implementation. For one, SQLAlchemy imposes no restrictions on the constructor of a class. An acceptable approach could be calling the __new__() method as SQLAlchemy does internally.

https://docs.sqlalchemy.org/en/latest/orm/constructors.html

Given that this code is only used in a write operation, I don't see speed as the primary concern.

@ticosax
Copy link
Contributor Author

ticosax commented Jan 14, 2019

Even if they claim they impose anything on __init__ they still initiate the attributes of the orm columns through it. When one create a new instance in the desired to insert it in the table.
https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/ext/declarative/base.py#L808

It means this is legit to expect __init__ to be called to instantiate a class from the Resource.create method.

SQLAlchemy bypasses __init__ when they load an object from the DB because they don't want to cause side effect added in user's __init__ when an object is simply read form the DB and mapped as a python object. This is 2 different use case in my opinion.

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.

3 participants