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

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

Merged
merged 27 commits into from
Mar 14, 2024

Conversation

j-carson
Copy link
Contributor

@j-carson j-carson commented Dec 19, 2023

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
Copy link

codecov-commenter commented Dec 19, 2023

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@j-carson
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

Caught up to main.

@mansenfranzen
Copy link
Owner

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
Copy link
Owner

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.

@mansenfranzen mansenfranzen added this to the v2.1 milestone Dec 27, 2023
@j-carson
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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

@mansenfranzen
Copy link
Owner

@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
Copy link
Owner

@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
Copy link
Owner

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.

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 this pull request may close these issues.

None yet

3 participants