eng: add option to put docstrings on model attributes BNCH-114718 (#225)

eli-bl · web-flow · commit 5f3b8ceb44cf · 2025-01-13T12:35:18.000-08:00
* eng: add option to put docstrings on model attributes BNCH-114718

* typo in comments

* also move docstrings to attributes in client classes
diff --git a/README.md b/README.md
@@ -100,6 +100,16 @@ class_overrides:
 
 The easiest way to find what needs to be overridden is probably to generate your client and go look at everything in the `models` folder.
 
+### docstrings_on_attributes
+
+By default, when `openapi-python-client` generates a model class, it includes a list of attributes and their
+descriptions in the docstring for the class. If you set this option to `true`, then the attribute descriptions
+will be put in docstrings for the attributes themselves, and will not be in the class docstring.
+
+```yaml
+docstrings_on_attributes: true
+```
+
 ### literal_enums
 
 By default, `openapi-python-client` generates classes inheriting for `Enum` for enums. It can instead use `Literal` 
diff --git a/end_to_end_tests/functional_tests/generated_code_execution/test_docstrings.py b/end_to_end_tests/functional_tests/generated_code_execution/test_docstrings.py
@@ -1,9 +1,11 @@
+import re
 from typing import Any, List
 
 from end_to_end_tests.functional_tests.helpers import (
     with_generated_code_import,
     with_generated_client_fixture,
 )
+from end_to_end_tests.generated_client import GeneratedClientContext
 
 
 class DocstringParser:
@@ -36,18 +38,57 @@ def get_section(self, header_line: str) -> List[str]:
       required: ["reqStr", "undescribedProp"]
 """)
 @with_generated_code_import(".models.MyModel")
-class TestSchemaDocstrings:
+class TestSchemaDocstringsDefaultBehavior:
     def test_model_description(self, MyModel):
         assert DocstringParser(MyModel).lines[0] == "I like this type."
 
-    def test_model_properties(self, MyModel):
+    def test_model_properties_in_model_description(self, MyModel):
         assert set(DocstringParser(MyModel).get_section("Attributes:")) == {
             "req_str (str): This is necessary.",
             "opt_str (Union[Unset, str]): This isn't necessary.",
             "undescribed_prop (str):",
         }
 
 
+@with_generated_client_fixture(
+"""
+components:
+  schemas:
+    MyModel:
+      description: I like this type.
+      type: object
+      properties:
+        prop1:
+          type: string
+          description: This attribute has a description
+        prop2:
+          type: string  # no description for this one
+      required: ["prop1", "prop2"]
+""",
+config="docstrings_on_attributes: true",
+)
+@with_generated_code_import(".models.MyModel")
+class TestSchemaWithDocstringsOnAttributesOption:
+    def test_model_description_is_entire_docstring(self, MyModel):
+        assert MyModel.__doc__.strip() == "I like this type."
+
+    def test_attrs_have_docstrings(self, generated_client: GeneratedClientContext):
+        # A docstring that appears after an attribute is *not* stored in __doc__ anywhere
+        # by the interpreter, so we can't inspect it that way-- but it's still valid for it
+        # to appear there, and it will be recognized by documentation tools. So we'll assert
+        # that these strings appear in the source code. The code should look like this:
+        #     class MyModel:
+        #         """I like this type."""
+        #         prop1: str
+        #         """This attribute has a description"""
+        #         prop2: str
+        # 
+        source_file_path = generated_client.output_path / generated_client.base_module / "models" / "my_model.py"
+        content = source_file_path.read_text()
+        assert re.search('\n *prop1: *str\n *""" *This attribute has a description *"""\n', content)
+        assert re.search('\n *prop2: *str\n *[^"]', content)
+
+
 @with_generated_client_fixture(
 """
 tags:
diff --git a/openapi_python_client/__init__.py b/openapi_python_client/__init__.py
@@ -95,6 +95,7 @@ def __init__(
 
         self.env.filters.update(TEMPLATE_FILTERS)
         self.env.globals.update(
+            config=config,
             utils=utils,
             python_identifier=lambda x: utils.PythonIdentifier(x, config.field_prefix),
             class_name=lambda x: utils.ClassName(x, config.field_prefix),
diff --git a/openapi_python_client/config.py b/openapi_python_client/config.py
@@ -41,6 +41,7 @@ class ConfigFile(BaseModel):
     package_version_override: Optional[str] = None
     use_path_prefixes_for_title_model_names: bool = True
     post_hooks: Optional[list[str]] = None
+    docstrings_on_attributes: bool = False
     field_prefix: str = "field_"
     generate_all_tags: bool = False
     http_timeout: int = 5
@@ -70,6 +71,7 @@ class Config:
     package_version_override: Optional[str]
     use_path_prefixes_for_title_model_names: bool
     post_hooks: list[str]
+    docstrings_on_attributes: bool
     field_prefix: str
     generate_all_tags: bool
     http_timeout: int
@@ -111,6 +113,7 @@ def from_sources(
             package_version_override=config_file.package_version_override,
             use_path_prefixes_for_title_model_names=config_file.use_path_prefixes_for_title_model_names,
             post_hooks=post_hooks,
+            docstrings_on_attributes=config_file.docstrings_on_attributes,
             field_prefix=config_file.field_prefix,
             generate_all_tags=config_file.generate_all_tags,
             http_timeout=config_file.http_timeout,
diff --git a/openapi_python_client/templates/client.py.jinja b/openapi_python_client/templates/client.py.jinja
@@ -5,6 +5,31 @@ from attrs import define, field, evolve
 import httpx
 
 
+{% set attrs_info = {
+    "raise_on_unexpected_status": namespace(
+        type="bool",
+        default="field(default=False, kw_only=True)",
+        docstring="Whether or not to raise an errors.UnexpectedStatus if the API returns a status code"
+            " that was not documented in the source OpenAPI document. Can also be provided as a keyword"
+            " argument to the constructor."
+    ),
+    "token": namespace(type="str", default="", docstring="The token to use for authentication"),
+    "prefix": namespace(type="str", default='"Bearer"', docstring="The prefix to use for the Authorization header"),
+    "auth_header_name": namespace(type="str", default='"Authorization"', docstring="The name of the Authorization header"),
+} %}
+
+{% macro attr_in_class_docstring(name) %}
+{{ name }}: {{ attrs_info[name].docstring }}
+{%- endmacro %}
+
+{% macro declare_attr(name) %}
+{% set attr = attrs_info[name] %}
+{{ name }}: {{ attr.type }}{% if attr.default %} = {{ attr.default }}{% endif %}
+{% if attr.docstring and config.docstrings_on_attributes +%}
+"""{{ attr.docstring }}"""
+{%- endif %}
+{% endmacro %}
+
 @define
 class Client:
     """A class for keeping track of data related to the API
@@ -29,14 +54,14 @@ class Client:
         ``httpx_args``: A dictionary of additional arguments to be passed to the ``httpx.Client`` and ``httpx.AsyncClient`` constructor.
 {% endmacro %}
 {{ httpx_args_docstring() }}
+{% if not config.docstrings_on_attributes %}
 
     Attributes:
-        raise_on_unexpected_status: Whether or not to raise an errors.UnexpectedStatus if the API returns a
-            status code that was not documented in the source OpenAPI document. Can also be provided as a keyword
-            argument to the constructor.
+        {{ attr_in_class_docstring("raise_on_unexpected_status") | wordwrap(101) | indent(12) }}
+{% endif %}
     """
 {% macro attributes() %}
-    raise_on_unexpected_status: bool = field(default=False, kw_only=True)
+    {{ declare_attr("raise_on_unexpected_status") | indent(4) }}
     _base_url: str = field(alias="base_url")
     _cookies: dict[str, str] = field(factory=dict, kw_only=True, alias="cookies")
     _headers: dict[str, str] = field(factory=dict, kw_only=True, alias="headers")
@@ -147,20 +172,20 @@ class AuthenticatedClient:
     """A Client which has been authenticated for use on secured endpoints
 
 {{ httpx_args_docstring() }}
+{% if not config.docstrings_on_attributes %}
 
     Attributes:
-        raise_on_unexpected_status: Whether or not to raise an errors.UnexpectedStatus if the API returns a
-            status code that was not documented in the source OpenAPI document. Can also be provided as a keyword
-            argument to the constructor.
-        token: The token to use for authentication
-        prefix: The prefix to use for the Authorization header
-        auth_header_name: The name of the Authorization header
+        {{ attr_in_class_docstring("raise_on_unexpected_status") | wordwrap(101) | indent(12) }}
+        {{ attr_in_class_docstring("token") | indent(8) }}
+        {{ attr_in_class_docstring("prefix") | indent(8) }}
+        {{ attr_in_class_docstring("auth_header_name") | indent(8) }}
+{% endif %}
     """
 
 {{ attributes() }}
-    token: str
-    prefix: str = "Bearer"
-    auth_header_name: str = "Authorization"
+    {{ declare_attr("token") | indent(4) }}
+    {{ declare_attr("prefix") | indent(4) }}
+    {{ declare_attr("auth_header_name") | indent(4) }}
 
 {{ builders("AuthenticatedClient") }}
 {{ httpx_stuff("AuthenticatedClient", "self._headers[self.auth_header_name] = f\"{self.prefix} {self.token}\" if self.prefix else self.token") }}
diff --git a/openapi_python_client/templates/helpers.jinja b/openapi_python_client/templates/helpers.jinja
@@ -1,8 +1,10 @@
-{% macro safe_docstring(content) %}
+{% macro safe_docstring(content, omit_if_empty=False) %}
 {# This macro returns the provided content as a docstring, set to a raw string if it contains a backslash #}
+{% if (not omit_if_empty) or (content | trim) %}
 {% if '\\' in content -%}
 r""" {{ content }} """
 {%- else -%}
 """ {{ content }} """
 {%- endif -%}
+{% endif %}
 {% endmacro %}
diff --git a/openapi_python_client/templates/model.py.jinja b/openapi_python_client/templates/model.py.jinja
@@ -47,25 +47,34 @@ T = TypeVar("T", bound="{{ class_name }}")
         {{ model.example | string | wordwrap(112) | indent(12) }}
 
     {% endif %}
-    {% if model.required_properties or model.optional_properties %}
+    {% if (not config.docstrings_on_attributes) and (model.required_properties or model.optional_properties) %}
     Attributes:
     {% for property in model.required_properties + model.optional_properties %}
         {{ property.to_docstring() | wordwrap(112) | indent(12) }}
     {% endfor %}{% endif %}
 {% endmacro %}
 
+{% macro declare_property(property) %}
+{%- if config.docstrings_on_attributes and property.description -%}
+{{ property.to_string() }}
+{{ safe_docstring(property.description, omit_if_empty=True) | wordwrap(112) }}
+{%- else -%}
+{{ property.to_string() }}
+{%- endif -%}
+{% endmacro %}
+
 @_attrs_define
 class {{ class_name }}:
-    {{ safe_docstring(class_docstring_content(model)) | indent(4) }}
+    {{ safe_docstring(class_docstring_content(model), omit_if_empty=config.docstrings_on_attributes) | indent(4) }}
 
     {% for property in model.required_properties + model.optional_properties %}
     {% if property.default is none and property.required %}
-    {{ property.to_string() }}
+    {{ declare_property(property) | indent(4) }}
     {% endif %}
     {% endfor %}
     {% for property in model.required_properties + model.optional_properties %}
     {% if property.default is not none or not property.required %}
-    {{ property.to_string() }}
+    {{ declare_property(property) | indent(4) }}
     {% endif %}
     {% endfor %}
     {% if model.additional_properties %}
