Skip to content

feat: Remove duplicated docs strings when use_attribute_docstrings is used in pydantic >= 2.7 #276

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

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

mansenfranzen
Copy link
Owner

@mansenfranzen mansenfranzen commented Apr 25, 2024

Type

enhancement, bug_fix


Description

  • Introduced new properties to manage the addition of docstrings and descriptions to fields, preventing duplication.
  • Refactored existing methods to utilize these properties, enhancing code clarity and maintainability.
  • Added compatibility checks for Pydantic version 2.7 to ensure correct behavior across different versions.
  • Expanded tests to cover new functionality and ensure robustness.
  • Updated user documentation to reflect changes and guide users on new configuration options.

Changes walkthrough

Relevant files
Enhancement
autodocumenters.py
Refactor Field Documentation Handling and Prevent Duplication

sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py

  • Added properties needs_doc_string and needs_description to handle
    docstring and description addition logic.
  • Refactored add_content method to use the new properties for
    conditional logic.
  • Improved methods for fetching and comparing field descriptions and
    docstrings to prevent duplication.
  • +50/-18 
    compatibility.py
    Add Pydantic Version Compatibility Check                                 

    tests/compatibility.py

  • Added a new function pydantic_ge_27 to check Pydantic version
    compatibility.
  • Imported necessary modules for version checking.
  • +12/-4   
    Tests
    configuration.py
    Extend Test Models for Documentation Behavior                       

    tests/roots/test-base/target/configuration.py

  • Added new test models to simulate different documentation behaviors
    based on Pydantic configuration.
  • +26/-0   
    test_configuration_fields.py
    Add Tests for Field Documentation Configurations                 

    tests/test_configuration_fields.py

  • Added extensive tests to verify the behavior of field documentation
    under various configurations.
  • +169/-2 
    test_edgecases.py
    Update Tests to Reflect New Field Description Handling     

    tests/test_edgecases.py

  • Adjusted test expectations to align with the new handling of field
    descriptions.
  • +0/-1     
    Documentation
    configuration.rst
    Update Documentation for Field Docstring Handling               

    docs/source/users/configuration.rst

  • Updated documentation to explain new handling of field docstrings and
    descriptions, especially regarding duplication.
  • +16/-5   

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @mansenfranzen mansenfranzen merged commit 0ddb12f into main Apr 25, 2024
    @mansenfranzen mansenfranzen deleted the fix_duplicated_field_docs branch April 25, 2024 14:26
    @github-actions github-actions bot added enhancement New feature or request bug_fix labels Apr 25, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (7e84765)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple complex changes across different files, including logic for handling docstrings and descriptions, compatibility checks for Pydantic version, and modifications in test cases. Understanding the implications of these changes requires a deep understanding of the existing codebase and the behavior changes introduced.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The method _get_pydantic_sanitized_doc_string in autodocumenters.py might not handle cases where docstrings contain only whitespace or special characters, which could lead to incorrect behavior when checking for identical descriptions and docstrings.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesphinxcontrib/autodoc_pydantic/directives/autodocumenters.py
    suggestion      

    Consider adding a check for empty or whitespace-only docstrings in _get_pydantic_sanitized_doc_string to ensure that these cases are handled correctly. This will prevent issues where empty or irrelevant docstrings are considered identical to descriptions, leading to unexpected documentation output. [important]

    relevant linedocstring = docstrings[0] # first element is always the docstring


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Bug
    Prevent potential IndexError by checking the array length before slicing.

    Refactor the method _get_pydantic_sanitized_doc_string to avoid slicing the docstring
    array without checking its length. This can lead to an IndexError if the docstring is
    empty. Instead, check if the docstring array has elements before accessing them.

    sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py [862-863]

    -docstring = docstrings[0]  # first element is always the docstring
    -without_last = docstring[:-1]  # last element is always empty
    +if docstrings:
    +    docstring = docstrings[0]  # first element is always the docstring
    +    without_last = docstring[:-1]  # last element is always empty
    +else:
    +    return ''
     
    Performance
    Optimize string manipulation for better performance.

    Use a more efficient method for replacing empty strings with line breaks in
    _get_pydantic_sanitized_doc_string. Instead of using a list comprehension for every
    element, directly join the elements and replace the empty ones in a single operation.

    sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py [864]

    -substitute_linebreaks = ['\n\n' if x == '' else x for x in without_last]
    +substitute_linebreaks = ''.join(without_last).replace('\n\n', '\n')
     
    Reduce redundant calls to improve performance and maintainability.

    The method needs_description performs multiple calls to
    self.pydantic.options.get_value('field-doc-policy'). This can be optimized by storing the
    result in a variable and reusing it.

    sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py [797-800]

     doc_policy = self.pydantic.options.get_value('field-doc-policy')
    -is_enabled = doc_policy in (
    -    OptionsFieldDocPolicy.BOTH,
    -    OptionsFieldDocPolicy.DESCRIPTION,
    -)
    +is_enabled = doc_policy in {OptionsFieldDocPolicy.BOTH, OptionsFieldDocPolicy.DESCRIPTION}
     
    Avoid unnecessary computation by conditionally retrieving data.

    In the add_description method, move the retrieval of the description variable inside the
    conditional check to avoid unnecessary computation when description is not used.

    sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py [870-871]

     description = self._get_field_description()
    -tabsize = self.directive.state.document.settings.tab_width
    -lines = prepare_docstring(description, tabsize=tabsize)
    +if description:
    +    tabsize = self.directive.state.document.settings.tab_width
    +    lines = prepare_docstring(description, tabsize=tabsize)
     
    Maintainability
    Simplify logical expression for clarity and maintainability.

    In the needs_description method, the check for identical_doc and self.needs_doc_string can
    be simplified by directly returning the result of the condition, instead of using an
    if-else structure.

    sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py [806-807]

    -identical_doc = description == self._get_pydantic_sanitized_doc_string()
    -is_duplicated = identical_doc and self.needs_doc_string
    +return identical_doc and self.needs_doc_string
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

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

    Successfully merging this pull request may close these issues.

    1 participant