Skip to content

Conversation

kc0506
Copy link
Contributor

@kc0506 kc0506 commented Aug 12, 2024

Change Summary

As per microsoft/pylance-release#6253, attribute docstrings are recommended to be placed in the top level of the class. I added several changes to #10104 to make tests pass.

The only tests that need to be modified are in test_type_hints, because typing.get_type_hints does not have access to types imported inside if TYPE_CHECKING blocks. Otherwise, we will have to import pydantic_core and pydantic.fields in pydantic.main only for type hint, which seems not ideal.

Related issue number

#10104

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Aug 12, 2024
Copy link

codspeed-hq bot commented Aug 12, 2024

CodSpeed Performance Report

Merging #10110 will not alter performance

Comparing kc0506:reorganize-basemodel-attributes (c988e2e) with main (58abc3d)

Summary

✅ 17 untouched benchmarks

@Viicos Viicos self-requested a review August 12, 2024 15:21
@kc0506
Copy link
Contributor Author

kc0506 commented Aug 12, 2024

Coudn't figure out why tests for python 3.8 failed 👀
Followed every steps from ci.yml in my python 3.8 virtualenv but no error occurs

(pydantic-3.8) (base) kc0506@kc0506:~/pydantic$ make test
PDM, version 2.17.3
pdm run coverage run -m pytest --durations=10
======================================================= test session starts =======================================================
platform linux -- Python 3.8.18, pytest-8.3.2, pluggy-1.5.0
codspeed: 2.2.1 (callgraph: not supported)
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=True warmup_iterations=100000)
rootdir: /home/kc0506/pydantic
configfile: pyproject.toml
testpaths: tests
plugins: benchmark-4.0.0, Faker-26.1.0, examples-0.0.12, mock-3.14.0, pretty-1.2.0, codspeed-2.2.1
collected 5814 items / 1 skipped        

@sydney-runkle
Copy link
Contributor

Looks similar to https://github.com/pydantic/pydantic/pull/10104/files, which @Viicos closed. Let's get his feedback here given that he was recently working on this 👍

Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

Thanks for giving a shot at this. I decided to close the previous PR as I saw it would require too much changes for a small added value.

However, I think I like the approach taken here.

Note that this is not a full review, I'll go a bit more in depth tomorrow morning.


Regarding the failing test, this is because Python 3.10 changed the way __annotations__ works. Prior to 3.10, __annotations__ would inherit the defined annotations from the base(s) class(es).

I'll take a look at a hopefully easy way to handle this.

@Viicos Viicos changed the title Reorganize BaseModel attributes Reorganize BaseModel annotations Aug 13, 2024
kc0506 and others added 5 commits August 13, 2024 14:47
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
@kc0506
Copy link
Contributor Author

kc0506 commented Aug 13, 2024

I forgot we could just clear BaseModel.__annotations__ at runtime 😅
Seems much cleaner than manually excluding BaseModel when evaluating type notations.

Copy link
Contributor

github-actions bot commented Aug 13, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic
  main.py
  pydantic/_internal
  _fields.py
  _model_construction.py
  _typing_extra.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

Thanks this is looking great. A few more comments. Would be awesome if you could update the description in the Attributes: section as well.

Ideally we should avoid the duplication but we seem to be limited by how mkdocstrings-python handle things :/

@sydney-runkle sydney-runkle added the awaiting author revision awaiting changes from the PR author label Aug 13, 2024
kc0506 and others added 2 commits August 16, 2024 11:32
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
@kc0506
Copy link
Contributor Author

kc0506 commented Aug 16, 2024

@Viicos Thanks for the detailed suggestions!

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Great work! Thanks @Viicos for giving some detailed line by line feedback.

Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

Awesome @kc0506

@Viicos Viicos merged commit 9e3d461 into pydantic:main Aug 16, 2024
59 checks passed
@kc0506 kc0506 deleted the reorganize-basemodel-attributes branch October 14, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants