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

run stubtest on _pydantic_core.pyi #702

Merged
merged 2 commits into from
Jun 28, 2023
Merged

run stubtest on _pydantic_core.pyi #702

merged 2 commits into from
Jun 28, 2023

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Jun 27, 2023

Change Summary

This PR adds use of mypy's stubtest tool to validate the .pyi file for pydantic-core, and fixes the errors detected.

Most of this is mechanical fixups which are good to have correct but not interesting to debate. The interesting things of note:

  • Two errors related to methods I didn't want in the stub, see TODO notes in .mypy-stubtest-allowlist.
  • I moved the TypedDict definitions to pydantic_core/__init__.py as these were reported at missing at runtime, and I think it's fair users may want to import these.
  • Many classes marked @final, because PyO3 types are final by default. We should review if we want to allow subtyping any of them.

Related issue number

N/A

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

@davidhewitt
Copy link
Contributor Author

please review

@codecov-commenter
Copy link

Codecov Report

Merging #702 (db90671) into main (c1c27d4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #702   +/-   ##
=======================================
  Coverage   93.73%   93.74%           
=======================================
  Files          99       99           
  Lines       13806    13821   +15     
  Branches       25       25           
=======================================
+ Hits        12941    12956   +15     
  Misses        859      859           
  Partials        6        6           
Impacted Files Coverage Δ
pydantic_core/__init__.py 88.23% <100.00%> (+6.41%) ⬆️
src/argument_markers.rs 87.09% <100.00%> (+0.21%) ⬆️
src/url.rs 99.59% <100.00%> (+<0.01%) ⬆️
src/validators/mod.rs 98.03% <100.00%> (+<0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c27d4...db90671. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 27, 2023

CodSpeed Performance Report

Merging #702 dh/mypy-stubtest (ff6b5cb) will improve performances by 11.87%.

Summary

🔥 1 improvements
❌ 0 regressions
✅ 124 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

Benchmarks breakdown

Benchmark main dh/mypy-stubtest Change
🔥 test_set_of_ints_core_json_duplicates 5.4 ms 4.8 ms +11.87%

@davidhewitt davidhewitt force-pushed the dh/mypy-stubtest branch 2 times, most recently from 40b03e9 to 62ab58d Compare June 27, 2023 15:16
@davidhewitt
Copy link
Contributor Author

Lint error is annoying consequence of installed wheel not placing native module inside the source tree, I'll push a proposed fix.

pydantic_core/_pydantic_core.pyi Outdated Show resolved Hide resolved
class PydanticSerializationError(ValueError):
def __init__(self, message: str) -> None: ...

@final
class PydanticSerializationUnexpectedValue(ValueError):
def __init__(self, message: 'str | None' = None) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should make these __new__ since that's what's actually happening? Can be another PR but I'd be okay if you just do it here since you're correctifying things already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably more correct, however I'd like to check how that influences mypy's handling of these types so I'll follow up in another PR.

src/validators/mod.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Contributor Author

Rebased on #705, which should make CI green.

example_message_python: str
message_template_json: _NotRequired[str]
example_message_json: _NotRequired[str]
example_context: 'dict[str, str | int | float] | None'
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not add from __future__ import annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None, let's do that.

@davidhewitt davidhewitt merged commit 703b7b2 into main Jun 28, 2023
@davidhewitt davidhewitt deleted the dh/mypy-stubtest branch June 28, 2023 15:54
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.

3 participants