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

adding docstrings #795

Merged
merged 5 commits into from
Jul 24, 2023
Merged

adding docstrings #795

merged 5 commits into from
Jul 24, 2023

Conversation

samuelcolvin
Copy link
Member

WIP

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #795 (eb8ffe3) into main (608db7b) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head eb8ffe3 differs from pull request most recent head 931fbd3. Consider uploading reports for the commit 931fbd3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #795      +/-   ##
==========================================
- Coverage   93.52%   93.48%   -0.04%     
==========================================
  Files         100      102       +2     
  Lines       14379    14606     +227     
  Branches       25       25              
==========================================
+ Hits        13448    13655     +207     
- Misses        925      945      +20     
  Partials        6        6              
Impacted Files Coverage Δ
python/pydantic_core/__init__.py 92.59% <100.00%> (+4.02%) ⬆️

... and 11 files with indirect coverage changes


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 608db7b...931fbd3. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 21, 2023

CodSpeed Performance Report

Merging #795 will not alter performance

Comparing docstrings (931fbd3) with main (608db7b)

Summary

✅ 126 untouched benchmarks

🆕 9 new benchmarks

Benchmarks breakdown

Benchmark main docstrings Change
🆕 test_uuid_from_string_core N/A 59.1 µs N/A
🆕 test_uuid_from_string_pyd N/A 87.7 µs N/A
🆕 test_uuid_from_uuid_core N/A 14.7 µs N/A
🆕 test_uuid_from_uuid_pyd N/A 15.8 µs N/A
🆕 test_core_python N/A 40.1 µs N/A
🆕 test_model_core_json N/A 85.9 µs N/A
🆕 test_core_raw N/A 14.7 µs N/A
🆕 test_core_str N/A 59.6 µs N/A
🆕 test_uuid N/A 67 µs N/A

Comment on lines +262 to +263
include: A set of fields to include, if `None` all fields are included.
exclude: A set of fields to exclude, if `None` no fields are excluded.
Copy link
Contributor

Choose a reason for hiding this comment

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

The types of include and exclude are _IncEx - we may want to make that alias public and document it.

image

__match_args__ = ('value',)

@property
def value(self) -> _T: ...
def value(self) -> _T:
Copy link
Contributor

Choose a reason for hiding this comment

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

The type annotation is _T, maybe we should rename to T for nicer docs?

image

Comment on lines +82 to +89
def __new__(cls, schema: CoreSchema, config: CoreConfig | None = None) -> Self:
"""
Create a new SchemaValidator.

Arguments:
schema: The [`CoreSchema`][pydantic_core.core_schema.CoreSchema] to use for validation.
config: Optionally a [`CoreConfig`][pydantic_core.core_schema.CoreConfig] to configure validation.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like __new__ doesn't show in the docs. @Kludex also reported an issue with __new__ and __init__ mismatching arguments in some cases (e.g. for exceptions, __init__ comes from BaseException). I think this is probably a PyO3 bug, PyO3 sets __new__ but not __init__.

Maybe for now it's more practical to set both __new__ and __init__ here, document __init__, and I'll have a think about how to resolve this in PyO3?

@samuelcolvin samuelcolvin marked this pull request as ready for review July 24, 2023 16:42
@samuelcolvin
Copy link
Member Author

going to merge this in the hope of including it in the next release, I'll fix those things in a new PR.

@samuelcolvin samuelcolvin merged commit 7802821 into main Jul 24, 2023
@samuelcolvin samuelcolvin deleted the docstrings branch July 24, 2023 16:43
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.

2 participants