DOC: Change doc template to fix SA04 errors in docstrings #28792 #32972
Conversation
|
Thanks @dilex42. My preference would be, instead of just listing the reverse of each method ( It'll be a bit long, but I don't think too much. I think all them are relevant for the users. And it'll reduce the complexity of that function, instead of increasing it. I guess it makes sense to have two, one for comparisons, and one for algebra operations. What do you think? |
|
I don't have experience to comment from designing perspective to what approach better. I guess having all functions listed is reasonable. But if I understand correctly there will be 2 static templates, additional |
|
Can't really tell without spending a decent amount of time where those docstrings are being used. I think for dataframe docstrings there are different templates, I thought it was the same for series. In that case my preferred option would be to have arithmetic and comparison methods all together in the See Also. @WillAyd do you have an opinion here? |
| See Also | ||
| -------- | ||
| Series.{reverse} | ||
| Series.{reverse} : {see_also_desc}. |
There was a problem hiding this comment.
For now, can you undo the rest of the changes, and just leave something generic like this?
| Series.{reverse} : {see_also_desc}. | |
| Series.{reverse} : Reverse of the operator, see `Python documentation <https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types>__` for more details. |
This fixes the docstring validation problem, adds useful information to the user, and keeps things simple.
There was a problem hiding this comment.
There will still be a problem with comparision operators rendering Series.None. I really can't see a way to handle both algebraic and comparision operators with one template.
|
The way I see it, we need to answer can we make it work with single flex template? If yes then sure let's do that. But if not then let's find the best solution with two templates. |
pandas/core/ops/docstrings.py
Outdated
| doc_see_also = _see_also_reverse_SERIES.format( | ||
| reverse=op_desc["reverse"], see_also_desc=op_desc["see_also_desc"], | ||
| ) | ||
| doc_no_examples = doc_no_examples + doc_see_also |
There was a problem hiding this comment.
I think the examples part of the doc building is already to complex to mix the see also in here.
Can you move this condition after line 13, and have something like:
if op_desc["reverse"]:
base_doc += _see_also_reverse_SERIES.format(...)I think this will be clearer this way.
pandas/core/ops/docstrings.py
Outdated
| @@ -352,6 +356,12 @@ def _make_flex_doc(op_name, typ): | |||
| if reverse_op is not None: | |||
| _op_descriptions[reverse_op] = _op_descriptions[key].copy() | |||
| _op_descriptions[reverse_op]["reverse"] = key | |||
| _op_descriptions[key][ | |||
| "see_also_desc" | |||
| ] = f"Reversed {_op_descriptions[key]['desc']}" | |||
There was a problem hiding this comment.
Do you mind writing a more descriptive description? I don't think for a user being shown radd for the first time, telling them Reverse add is very helpful. I wrote an example with a link in a previous comment that you can use as reference.
pandas/core/ops/docstrings.py
Outdated
| ] = f"Reversed {_op_descriptions[key]['desc']}" | ||
| _op_descriptions[reverse_op][ | ||
| "see_also_desc" | ||
| ] = f"Usual {_op_descriptions[key]['desc']}" |
There was a problem hiding this comment.
I think this description can also be improved.
|
Is this description and formatting good? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @dilex42 |
As requested in #32823 creating separate PR.
Also all descriptions for methods starts with upper case and it looks a little out of place. Should it be casted to lowercase perhaps?