Closed mehdisadeghi closed 7 years ago
Reviews are welcome.
@mehdisadeghi Maybe we actually don't want to set data
in __init__
?
The problem here is that data may conflict with the other arguments in __init__
.
I personally prefer named arguments to thedata
argument because data
is generic and obscure. However if we must keep data
(hope not) then I think the other arguments should go.
@kitchoi I agree with your argument. This PR is a quick fix to a problem that I just observed in a rather old code that uses data
parameter. I have to discuss a little bit internally to see how we intend to instantiate classes and update this thoroughly. I'll do it separately.
@mehdisadeghi Can the rather old code be changed? ;) Something like cuds_item=CUDSItem(..); cuds_item.data.update(new_data)
Can the rather old code be changed? ;) Something like cuds_item=CUDSItem(..); cuds_item.data.update(new_data)
That's orthogonal to this PR, btw.
Can the rather old code be changed? ;)
Of course it can, code is subject to change! :)
Regarding your earlier comment, I think we should replace this hack with a better fix. If we remove the data
property and restrict the init params to keyword arguments only, then users will be forced to call update
on data:
m = Material(name='mymaterial')
m.data.update({CUBA.MASS: random.uniform(1.0, 2.0)})
If they otherwise do an assignment, they will lose (unintentionally) name
and description
and other default params, if any:
m = Material(name='mymaterial')
m.data = {CUBA.MASS: random.uniform(1.0, 2.0)} # `name` is gone.
AFAIK, this behavior is unexpected. (the principle of least surprise) Let's see if we can come up with a good idea to fix this. I'll come back to this again.
@mehdisadeghi this PR is no longer relevant here. The generator code has been moved to simphony-common
When the
data
named parameter is passed to the constructor of any generated class, it will replace the internaldata
, no matter whether it provides all the necessary keywords such asCUBA.NAME
.Solution: update internal
data
instead of replacing it.