siesta-project / aiida_siesta_plugin

Source code for the AiiDA-Siesta package (plugin and workflows). See wiki
Other
6 stars 11 forks source link

Changes in the tkdict module. #54

Closed bosonie closed 4 years ago

bosonie commented 4 years ago

Changes:

Let me know if this is ok in your opinion, @vdikan and @pfebrer96

pfebrer commented 4 years ago

I changed the get_last_key to get_untranslated_key, that clarifies more the scope of the method.

I think the name get_last_key indicates that you are only going to get the last key that you attempted to set, which might be important to note. get_last_untranslated_key would be the correct name, but it may be to large :)

Is it safe to assume that most of the times you will want the translated dict? Then you could have get_dict = translated_dict or even dict = property(fget=translated_dict) or value = property(fget=translated_dict). I like to get the "hidden value" of an object with obj.value because it's general and I know that it will work accross different objects (I don't have to know the specific attribute name for each object). This is something that bothers me with aiida nodes, but it may be only me hahaha.

bosonie commented 4 years ago

Thanks @pfebrer96, I think get_last_untranslated_key is fine. Also to name dict() the translated_dict() is fine with me. Regarding the fact to make it a property, I was recently told that when you construct a resource, it is suggested to use a method and not a property. Let's see what @vdikan thinks.

pfebrer commented 4 years ago

Also to name dict() the translated_dict() is fine with me. Regarding the fact to make it a property, I was recently told that when you construct a resource, it is suggested to use a method and not a property. Let's see what @vdikan thinks.

If it's a method instead of a property, then get_* is a better name isn't it? Maybe it's just my own opinion. It would also be consistent with aiida dicts.

vdikan commented 4 years ago

In general the code looks good to me, with the behavior that is expected from Dict-like object. I spotted one difference, that TKDict.items() returns list and not a dict_items class instance. But I do not think it matters. When it does, this type can be cast with TKDict.dict().items()

bosonie commented 4 years ago

Thanks @vdikan and @pfebrer96 for the feedback. In the last commit:

I had a look on how to return a dict_items, but I couldn't find the way. For the moment I leave it like that, but if you know how to fix it, let me know!!!