Fix for issue 137 - cannot compare between <type> and None when sorting fields

[j-carson]

Hello,

I think I have a proposed fix for #137. However, I am having problems running the tox tests due to problems with erdantic. I have tried

poetry run tox -e py310-pydanticlatest-sphinxlatest-no_erdantic

but it is still failing on trying to install erdantic and not finding include files for that.

Please suggest workaround?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.87342% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 95.79%. Comparing base (4248f52) to head (05bf661).

Files	Patch %	Lines
...rib/autodoc_pydantic/directives/autodocumenters.py	89.47%	8 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   95.86%   95.79%   -0.07%     
==========================================
  Files          12       12              
  Lines        1040     1095      +55     
==========================================
+ Hits          997     1049      +52     
- Misses         43       46       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[j-carson]

Hmmm... I see lots of failed tests around

  AttributeError: 'PosixPath' object has no attribute 'rmtree'

At first glance, I don't see how my change could have caused this error, but please advise.

[mansenfranzen]

Hmmm... I see lots of failed tests around

  AttributeError: 'PosixPath' object has no attribute 'rmtree'

At first glance, I don't see how my change could have caused this error, but please advise.

@j-carson Thanks for your continuous contributions!

The PosixPath issue first appeared with Sphinx 7.2 and it is fixed on the main branch. If you rebase with main, you should no longer face this issue.

[j-carson]

Caught up to main.

[mansenfranzen]

Hello,

I think I have a proposed fix to issue 137. However, I am having problems running the tox tests due to problems with erdantic. I have tried

poetry run tox -e py310-pydanticlatest-sphinxlatest-no_erdantic

but it is still failing on trying to install erdantic and not finding include files for that.

Please suggest workaround?

Erdantic requires (py)graphviz to be installed on your system. This can be tricky, especially on windows. See here for more.

[mansenfranzen]

As mentioned in #137, we will require test cases to reliably fix the incorrect behaviour. If you need any support setting up tests with autodoc_pydantic please let me know. Testing sphinx extensions are not trivial. I'm more than happy to set up the test case given a minimal reproducible example.

[j-carson]

but it is still failing on trying to install erdantic and not finding include files for that. Please suggest workaround?

Erdantic requires (py)graphviz to be installed on your system. This can be tricky, especially on windows. See here for more.

I installed graphviz, but pygrahviz is just giving me fits. I see that there is a no_erdantic option in the tox config -- Can you help me with the right tox incantation to run tests without a successful pygraphviz install?

[mansenfranzen]

I had to look up the correct invocation for tox, too. You can list all available environments defined in the tox.ini via tox -l. To execute tests without erdantic, you can use tox -e no_erdantic. Just tested it on a windows os without graphviz available and it worked fine.

[mansenfranzen]

I added a test case that at least covers the incorrect sorting behavior with inherited models.

[j-carson]

Thanks for the test case, but I'm not sure why the expected test result shows "field_b" and "field_a" (and their validators) if autodoc's "inherited-members" option is not turned on?

I'm checking for the inherited-members option as is done at https://github.com/mansenfranzen/autodoc_pydantic/blob/1d14f120373e481023de889711210f4d2a2b853c/sphinxcontrib/autodoc_pydantic/directives/autodocumenters.py#L163

[mansenfranzen]

You are completely right, the inherited member option has to be turned on in the test case.

[j-carson]

Another edge case question:

test_audotdoc_pydantic_model_show_validator_summary_inherited_without_inherited

The classes are

class ModelShowValidatorsSummary(BaseModel):
    """ModelShowValidatorsSummary."""

    field: int = 1

    @field_validator("field")
    def check(cls, v) -> str:
        return v

class ModelShowValidatorsSummaryInherited(ModelShowValidatorsSummary):
    """ModelShowValidatorsSummaryInherited."""

    @field_validator("field")
    def check_inherited(cls, v) -> str:
        return v

If "inherited-members" is not set, the test suite thinks it wants this result:

result = [
        '',
        '.. py:pydantic_model:: ModelShowValidatorsSummaryInherited',
        '   :module: target.configuration',
        '',
        '   ModelShowValidatorsSummaryInherited.',
        '',
        '   :Validators:',
        '      - :py:obj:`check <target.configuration.ModelShowValidatorsSummary.check>` » :py:obj:`field <target.configuration.ModelShowValidatorsSummaryInherited.field>`',
        '      - :py:obj:`check_inherited <target.configuration.ModelShowValidatorsSummaryInherited.check_inherited>` » :py:obj:`field <target.configuration.ModelShowValidatorsSummaryInherited.field>`',
        ''
    ]

However, my code considers the "check" method to be an inherited validator, and squashes it when inherited-members is not turned on. Can you explain better why "check" should be included here?

@mansenfranzen is it OK to edit the expected answer for above test?

[j-carson]

@mansenfranzen I'm working my way through the build failures... For sphinx 4.5, this is the error

E sphinx.errors.VersionRequirementError: The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version.

Do you happen to know how to tell sphinx not to include the applehelp extension for the older sphinx versions?

Update: Above issue in sphinx repo - but no fix - https://github.com/sphinx-doc/sphinx/issues/11890

[j-carson]

Update - I fixed the sphinx 4.X install problem by adding dependencies (based on the github issue linked) to the tox.ini file. The last problem on the current test suite is a stinker -- pydantic20-sphinx53 passes on python 3.10 but not on python 3.9 but I'll keep digging at it.

[j-carson]

Note to self: Here is the root cause of why tests older than python 3.9 are failing https://docs.python.org/3/howto/annotations.html#accessing-the-annotations-dict-of-an-object-in-python-3-9-and-older

[j-carson]

@mansenfranzen The bug that was causing the last test failure is actually in the getannotations function in sphinx/util/inspect.py, function getannotations needs to check for python 3.9 and follow procedure in the above link.

I will need to attempt to become a sphinx contributor to fix that part, so for the purposes of this repository, I changed the test to accept either the right answer, or the wrong answer with the extra inherited validator if it's python 3.9 or earlier.

I now have all the existing tests passing, so I'd appreciate it if you started to review the code.... It turned out to be a LOT more than just one little line...

I think the only thing left is to look at the coverage reports to see if any more test cases are needed.

[j-carson]

The fix for the sphinx bug w/ python 3.9 that messes up some of the inheritance test cases has been merged https://github.com/sphinx-doc/sphinx/pull/11919

[mansenfranzen]

@j-carson Thx for putting all this effort in this PR and sorry for letting you wait for so long. I will review your PR in the next days.

[mansenfranzen]

@j-carson Once again thanks a lot for the work you've put into this PR! I merged main and added several tests that explicitly test inheritance, also including overwriting fields of parent classes because this seemed to be a relevant edge case, too.

By the way, you've actually fixed a bug that incorrectly showed/hided validators from the documentation. However, this bug has not been reported yet. The tests now cover the correct behavior. Additionally, you've fixed #178, too. Great work! I'm not completely done with the PR yet while I need to update the changelog and review some more tests.

[mansenfranzen]

Interestingly, the edge case for python < 3.10 only occurs if the child class does not have any fields. In contrast, if the child class has at least a single field defined, the incorrect behavior disappears. See here.