👌 IMPROVE: Make verdi group messages consistent

[CasperWA]

Closes #4998.

• Use label instead of name.
• Use group.__class__.__name__ always.
• Minor syntax fixes.

Edit: This PR has been updated according to the discussion below.

[codecov]

Codecov Report

Merging #4999 (ff3a7ab) into develop (d138842) will increase coverage by 0.01%.
The diff coverage is 73.92%.

@@             Coverage Diff             @@
##           develop    #4999      +/-   ##
===========================================
+ Coverage    80.17%   80.17%   +0.01%     
===========================================
  Files          515      515              
  Lines        36746    36741       -5     
===========================================
- Hits         29457    29453       -4     
+ Misses        7289     7288       -1     

Flag	Coverage Δ
django	74.65% <73.92%> (-<0.01%)	⬇️
sqlalchemy	73.57% <73.92%> (-<0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/groups.py	94.70% <50.00%> (+0.78%)	⬆️
aiida/cmdline/commands/cmd_group.py	90.16% <76.20%> (-0.11%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d138842...ff3a7ab. Read the comment docs.

[sphuber]

I am always a fan of consistency and I also like:

{group.__class__.__name__}<{group.label}>

as the standard representation of a Group instance. But to prevent us having to repeat this lengthy snippet each time, what about we make this the implementation of Group.__str__? Then we just put {group} in the string and presto. The current __str__ implementation includes the user name but I think that is used so little that we can remove it.

[CasperWA]

(...) But to prevent us having to repeat this lengthy snippet each time, what about we make this the implementation of Group.__str__? Then we just put {group} in the string and presto. The current __str__ implementation includes the user name but I think that is used so little that we can remove it.

Fine by me - if there are no objections to this? @giovannipizzi @ltalirz @chrisjsewell @mbercx ? (I will add a commit with this change as it's quick, which can then be reverted if there's a huge uprising of sorts.)

Edit: Looking at the code I have just realized you are mixing __repr__ and __str__.
__repr__ is somewhat equivalent to {group.__class__.__name__<{group.label>, while __str__ is "{label}" [type {type_string]/user-defined], of user {user.email}...
I guess we should just use __str__ as is now then in the messages above? Maybe removing the user part still?

[sphuber]

Edit: Looking at the code I have just realized you are mixing __repr__ and __str__.

Wasn't really mixing them up, but was just describing how I think the __str__ should be implemented. The __repr__ should also be improved. The general guideline for these two methods (which we aren't yet widely respecting but we started changing to) is that __str__ should return a user readable string and __repr__ should provide the necessary information for a developer to reliably identify the object. So I would actually propose the following for both methods:

def __repr__(self):
    return f'self.__class__.__name__<self.uuid>'

def __str__(self):
    return f'self.__class__.__name__<self.label>'

I forgot where, but we started using this definition in other places since some time ago. We just never got around to apply this to all important ORM classes.

[CasperWA]

(...) The general guideline for these two methods (which we aren't yet widely respecting but we started changing to) is that __str__ should return a user readable string and __repr__ should provide the necessary information for a developer to reliably identify the object. (...)

I don't completely agree with this, at least not the __repr__ part.
I believe the true return value for __repr__ should always be passable to the built-in eval() function to instantiate an object with similar attributes and make up as the instance that is represented. (A clear example of this is seen if you "represent" a str, it simply gets wrapped in quotations so it can be evaluated as a str.)

(...) So I would actually propose the following for both methods:

def __repr__(self):
    return f'self.__class__.__name__<self.uuid>'

def __str__(self):
    return f'self.__class__.__name__<self.label>'

This would then instead become:

def __repr__(self):
    return f'{self.__class__.__name__}(label={self.label}, user={self.user}, description={self.description}, type_string={self.type_string}, backend={self.backend})'

def __str__(self):
    return f'{self.__class__.__name__}<{self.label}>'

I'm not sure whether all the attributes here match up exactly with the initialization parameters --- but, yeah.

I forgot where, but we started using this definition in other places since some time ago. We just never got around to apply this to all important ORM classes.

Might be good to make a collective issue for this then? Low priority but still important.

[sphuber]

I believe the true return value for __repr__ should always be passable to the built-in eval() function to instantiate an object with similar attributes and make up as the instance that is represented.

I know that this is what is often written on this topic, but I don't think this is useful or necessary in practice. Note that even if you get the full signature correct (and even you yourself already admit here that you are not sure your current implementation is correct) there is no guarantee that you know where the class was imported from and so you cannot necessarily reconstruct the object using eval. This whole guideline was once conceived to express the goal that the repr should be as unambiguous as possible as to what object it represents. It was never intended that it should be possible to reconstruct the object. This is why I think in our use case (for ORM objects at least) we should simply use the UUID as these objects always have one and it will always be guaranteed to be unambiguous.

[CasperWA]

I believe the true return value for __repr__ should always be passable to the built-in eval() function to instantiate an object with similar attributes and make up as the instance that is represented.

I know that this is what is often written on this topic, but I don't think this is useful or necessary in practice. Note that even if you get the full signature correct (and even you yourself already admit here that you are not sure your current implementation is correct) there is no guarantee that you know where the class was imported from and so you cannot necessarily reconstruct the object using eval. This whole guideline was once conceived to express the goal that the repr should be as unambiguous as possible as to what object it represents. It was never intended that it should be possible to reconstruct the object. This is why I think in our use case (for ORM objects at least) we should simply use the UUID as these objects always have one and it will always be guaranteed to be unambiguous.

Okay. I think this is something that should be decided and implemented for all ORM classes in one go (separate PR) instead of changing it here. So I'll open an issue for this (if one doesn't already exist) and revert changes to __repr__, keeping the current change to __str__.

[chrisjsewell]

Discussion of the actua group str contruct has moved to #3087, so this is good to go thanks 👍