add magic method XCom docstrings D105

[okirialbert]

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[okirialbert]

Add XCom magic method docstrings #37523

[potiuk]

I find it odd that we need to describe the dunder methods (str ) - if that's the case then the rule should not be enabled.

It's simply wrong to describe what those methods do. Generally those descriptions, describe what is implemented in them (not what they do) - which is duplication and we should never try to attempt to describe in the docstring in plain English what is internal and plain visible in code - we should rather explain what is the intention of the methods (and we already know that because we know what the dunder methods in question do (__repr__, __eq_) . Repeating and explaining the algorithm used internally is really bad - because it's almost 100% certain that when we change the implementation (and for example add one more field in __repr__ ) we will forget to also update description and it will be just lying to us.

That makes value of those docstrings negative, because they will be misleading the user of those methods

[okirialbert]

I was striving for better readability by including detailed function descriptions but now I see how it can be misleading in the event of future changes.

[uranusjr]

Magic methods already have defined semantics and documenting them is entirely missing the point. If a tool requires you to do it, it is not a good tool.

[potiuk]

Magic methods already have defined semantics and documenting them is entirely missing the point. If a tool requires you to do it, it is not a good tool.

Yep. Agree with @uranusjr - we shoudl NOT be documenting those (dunder) methods at all.

[Taragolis]

My 2c, there is always depends on. E.g. why on earth we required to create __str__ and __repr__ for the classes:

• is this just for beatify?
• Or it has some specific cases, e.g.

  airflow/airflow/models/xcom_arg.py

  Lines 277 to 286 in c0b849a

  	def __str__(self) -> str:
  	"""
  	Backward compatibility for old-style jinja used in Airflow Operators.
  	**Example**: to use XComArg at BashOperator::
  	BashOperator(cmd=f"... { xcomarg } ...")
  	:return:
  	"""

__repr__ might have a specific meaning for the serialisation, e.g.

class Foo:
    ...

class Bar:
    def __repr__(self):
        return "BarSentinel"

e.g. Foo will return a new value each time it created <__main__.Foo object at 0x1011e5610> or in another time it would be <__main__.Foo object at 0x101065610>. So if this object might use into the serialisation, then it will be the reason why we overwrite SerializedDag every time, and simple "String representation for deterministic output" might help understand why it required to set this method.

The problem here that for some methods you have to define, e.g. if you subclassing of collections.abc, and that is pretty clear why you have to define and docstring in this case redundant

I have not clear vision, should we define it for all classes (rule enable) or it should remain on conscience and responsibility of contributor/reviewers (rule disable).

And there other question, do we have an idea why we originally (3 ago) select this rules #10742 . For example Personally I thought it is redundant to also enable D107 | Missing docstring in __init__, so maybe better review and discuss this rules again and remove redundant one?

[potiuk]

As discussed in #38452 - we remove the D105 rule from our checks.