Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Property :rtype: is redundant #286

Closed
hoodmane opened this issue Jan 13, 2023 · 5 comments · Fixed by #287
Closed

Property :rtype: is redundant #286

hoodmane opened this issue Jan 13, 2023 · 5 comments · Fixed by #287

Comments

@hoodmane
Copy link
Collaborator

hoodmane commented Jan 13, 2023

Looking at the output, it seems that properties have redundant types. In the test suite, a_property is an example. Defined like:

@property
def a_property(self) -> str:
    """
    Property docstring
    """

The test suite asserts that the type of the property is included in two places. This strikes me as redundant and slightly confusing: does a property really "return" a value? In my opinion, the abstraction makes it look like we are "accessing" the value rather than returning it. To be specific, I want the following change in the output.

     property a_property: str
  
        Property docstring
  
-       Return type:
-          "str"
- 

I applied the following patch to do this:

--- a/src/sphinx_autodoc_typehints/__init__.py
+++ b/src/sphinx_autodoc_typehints/__init__.py
@@ -611,7 +611,7 @@ def _inject_types_to_docstring(
 
                 lines.insert(insert_index, type_annotation)
 
-    if "return" in type_hints and not inspect.isclass(original_obj):
+    if "return" in type_hints and not inspect.isclass(original_obj) and not inspect.isdatadescriptor(original_obj):
         if what == "method" and name.endswith(".__init__"):  # avoid adding a return type for data class __init__
             return
         formatted_annotation = format_annotation(type_hints["return"], app.config)

Do people prefer the current behavior? If some people want the redundant property types, could we add a config option to opt out of it?

@gaborbernat
Copy link
Member

Which of these two is used by sphinx?

@hoodmane
Copy link
Collaborator Author

Both appear in rendered sphinx. I'll post a screenshot when I get to a computer.

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 13, 2023

The rst looks like:

   .. py:property:: FetchResponse.status_text
      :module: pyodide.http
      :type: str

      Response status text

      :rtype: str

Which is rendered as:

property status_text: str  <-- from :type: str

    Response status text

    Return type: str      <-- from :rtype: str

I think it looks better with :rtype: str removed and only :type: str included.

With both:

Screenshot from 2023-01-13 15-01-44

With just the :type: str:

Screenshot from 2023-01-13 15-05-56

@gaborbernat
Copy link
Member

Yeah let's remove the return type here, PR welcome.

@hoodmane
Copy link
Collaborator Author

Great, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants