Update DataTree #1042
Update DataTree #1042
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1042 +/- ##
==========================================
+ Coverage 88.39% 88.49% +0.10%
==========================================
Files 78 78
Lines 8907 8915 +8
==========================================
+ Hits 7873 7889 +16
+ Misses 1034 1026 -8 |
| sub_tree.__to_dict(sub_dic) | ||
| dic[sub_tree_name] = sub_dic | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
| def to_dict(self): | |
| @property | |
| def dict(self): |
You can make this a property
You could also define the __dict__ dunder method so that dict(your_object) works
There was a problem hiding this comment.
Also, I'd personally integrate __to_dict in the dict property, and then call directly the dict property in other parts of the code.
There was a problem hiding this comment.
@PProfizi Will do! Wasn't aware of this!
There was a problem hiding this comment.
@PProfizi Seems dict is meant for something else (e.g. object inspection).
|
|
||
| def to_dict(self): | ||
| """ | ||
| Returns a dictionary representation of the DataTree |
There was a problem hiding this comment.
How do you enforce the fact it is read-only if you return an actual Python dict?
There was a problem hiding this comment.
It was originally a comment from @cbellot000 meaning that modifying the attributes & attribute-names in the dict didn't actually modify anything the DataTree. Which on second thought, the fact that we return a dict makes it clear enough. But no harm in leaving there I think.
src/ansys/dpf/core/data_tree.py
Outdated
| return coll_obj.get_integral_entries() | ||
|
|
||
| @property | ||
| def get_sub_tree_names(self): |
There was a problem hiding this comment.
| def get_sub_tree_names(self): | |
| def sub_tree_names(self): |
src/ansys/dpf/core/data_tree.py
Outdated
| return out | ||
|
|
||
| @property | ||
| def get_attribute_names(self): |
There was a problem hiding this comment.
| def get_attribute_names(self): | |
| def attribute_names(self): |
@jose-henriquezroa
We try not to make properties with "get" or "set" in their names as is it already explicit that this is a getter.
There was a problem hiding this comment.
Also above, why not just call the methods to_json or from_txt without the read and write parts. This too seems explicit enough without it.
@PProfizi I missed this comment. I tried, the json written by the data-tree is not really conformant with the actual json format (might be a bug). It raises an error when casting it to a json object.
There was a problem hiding this comment.
Good point @PProfizi. In that I should implement an equivalent "attribute_name" with a setter decorator that raises an error when the users calls it, since the setter here shouldn't be implemented.
And yet I'm still not fully happy with this since it betrays a bit the expectation of the user. Because keeping the "get_" prefix makes it clear that its only a getter from the start.
There was a problem hiding this comment.
@jose-henriquezroa you do not have to implement the setter and make it raise an error if what you want is for people to not be able to set. Simply not specifying a setter is enough.
There was a problem hiding this comment.
@PProfizi You're right, my mistake. I misunderstood the "property".
Nonetheless, in the very particular case of the DataTree we've also defined the setattr function. Which means that if the user does data_tree.get_attribute_name = "hello", instead of throwing an error it's going to define an attribute in the data_tree object with key "get_attribute_name" and value "hello". So I guess using "properties" is too risky here still.
No description provided.