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

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

Merged
merged 3 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
Original file line number Diff line number Diff line change
@@ -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:
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions openapi_python_client/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ def __init__(

self.env.filters.update(TEMPLATE_FILTERS)
self.env.globals.update(
config=config,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the openapi-python-client config options available as a config variable in the Jinja templates.

utils=utils,
python_identifier=lambda x: utils.PythonIdentifier(x, config.field_prefix),
class_name=lambda x: utils.ClassName(x, config.field_prefix),
Expand Down
3 changes: 3 additions & 0 deletions openapi_python_client/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
51 changes: 38 additions & 13 deletions openapi_python_client/templates/client.py.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,31 @@ from attrs import define, field, evolve
import httpx


Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintainer pointed out on the upstream PR that there was a similar docstring issue in the generated client classes. So I updated this template in the upstream PR & also in this one. As with my other changes, these result in exactly the same code if you do not have the new option enabled - the proof of that is that I didn't need to make any changes in the "golden record" code under end_to_end_tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see what the generated client class looks like when the new option is set here - because, in the upstream PR, I did include the new feature in the "golden record"-based tests (since there are no functional tests on that branch).

{% 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
Expand All @@ -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")
Expand Down Expand Up @@ -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") }}
4 changes: 3 additions & 1 deletion openapi_python_client/templates/helpers.jinja
Original file line number Diff line number Diff line change
@@ -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 -%}
Copy link
Author

@eli-bl eli-bl Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I added the omit_if_empty parameter here is to avoid inserting an empty """ """ for the class docstring if the model class doesn't have any description. That wasn't really an issue with the previous behavior, because model classes almost always have properties so there was always a class docstring that included the property descriptions. I am not setting omit_if_empty in the template unless the new docstrings_on_attributes option is enabled, so that I can guarantee that the behavior is 100% unchanged if you don't set the option.

r""" {{ content }} """
{%- else -%}
""" {{ content }} """
{%- endif -%}
{% endif %}
{% endmacro %}
17 changes: 13 additions & 4 deletions openapi_python_client/templates/model.py.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Expand Down
Loading