DM-19873: Resurrect get() and add __getitem__ calling it #50
Conversation
There was a problem hiding this comment.
Overall this looks great.
I have two significant requests:
- Please add unit tests for get(key, default). I am sorry if they are present and I missed them.
- Please consider dropping
updateunless you really need it. PropertyList and PropertySet are only "sort of" dicts and I worry that the more methods are added to make them the same the more likely folks will be caught out by the differences.
Other than that I just had a few requests for expanded documentation.
| self.combine(addition) | ||
| else: | ||
| for k, v in addition.items(): | ||
| self[k] = v |
There was a problem hiding this comment.
I would be much happier if you omitted this unless you really need it. I think it goes a bit too far in treating PropertySet like a dict.
What does this do if the key already exists? Does this add the new value or replace the existing values? If you must have this then please be much more explicit about its behavior in the documentation.
There was a problem hiding this comment.
I will add more explanation. I added this because I had a direct need for it.
| @@ -501,6 +534,9 @@ def __setitem__(self, name, value): | |||
| value = ps | |||
| self.set(name, value) | |||
|
|
|||
| def __getitem__(self, name): | |||
| return self.getScalar(name) | |||
|
|
|||
There was a problem hiding this comment.
Please document what this does in a doc string, if you can figure out a sensible place to put it. I suspect for it to be easily understood it should be in the doc string for the class, but maybe a doc string here would work if sphinx is smart enough.
There was a problem hiding this comment.
daf_base still uses doxygen and I think the best place to document the dict-like behavior of PropertySet would be in the index.rst file once it's ported to sphinx. There doesn't seem to be a doxygen root page that I can augment.
get() now matches the dict.get API except that it always calls getScalar internally whilst also providing a default value. The tests have been modified accordingly and the deprecation message has been removed.
get() now matches the dict.get API except that it always
calls getScalar internally whilst also providing a default
value.
The tests have been modified accordingly and the deprecation
message has been removed.