Skip to content

Conversation

sydney-runkle
Copy link
Contributor

@sydney-runkle sydney-runkle commented Sep 18, 2024

Closes #10315

Decided not to warn on method overrides, as that's an incredibly common practice in Python, and type checking does enough warning here in cases of incompatibility.

@sydney-runkle sydney-runkle added the relnotes-change Used for changes to existing functionality which don't have a better categorization. label Sep 18, 2024
Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

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

1 files reviewed, 1 outstanding issue(s) found.

Copy link

cloudflare-workers-and-pages bot commented Sep 18, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 36a7816
Status: ✅  Deploy successful!
Preview URL: https://5749a93c.pydantic-docs.pages.dev
Branch Preview URL: https://protected-ns.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hq bot commented Sep 18, 2024

CodSpeed Performance Report

Merging #10441 will not alter performance

Comparing protected-ns (36a7816) with main (296f2ca)

Summary

✅ 38 untouched benchmarks

@sydney-runkle sydney-runkle force-pushed the protected-ns branch 2 times, most recently from 231eb93 to e9d3e43 Compare September 19, 2024 14:27
changing tests, adding updates

remove added space

Update docs/concepts/serialization.md

Co-authored-by: hyperlint-ai[bot] <154288675+hyperlint-ai[bot]@users.noreply.github.com>

Update docs/concepts/serialization.md

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

removing warning related code
removing unneeded warning check
Copy link
Contributor

github-actions bot commented Sep 19, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

one real question, otherwise LGTM.

Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

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

1 files reviewed, 2 total issue(s) found.

@sydney-runkle
Copy link
Contributor Author

sydney-runkle commented Oct 2, 2024

I think this should be ready for review. Given #10493, I actually don't think it's necessary that we add model_fields, model_extra, etc to the protected_namespaces default:

from pydantic import BaseModel
from typing import Any

class User(BaseModel):
    model_fields: list[str]
    model_computed_fields: list[str]
    model_extra: dict[str, Any]
    model_fields_set: set[str]

user = User(model_fields=['id', 'name'], model_computed_fields=[], model_extra={}, model_fields_set={'id', 'name'})
"""
/Users/programming/pydantic_work/pydantic/pydantic/_internal/_fields.py:190: UserWarning: Field name "model_fields" in "User" shadows an attribute in parent "BaseModel"
  warnings.warn(
/Users/programming/pydantic_work/pydantic/pydantic/_internal/_fields.py:190: UserWarning: Field name "model_computed_fields" in "User" shadows an attribute in parent "BaseModel"
  warnings.warn(
/Users/programming/pydantic_work/pydantic/pydantic/_internal/_fields.py:190: UserWarning: Field name "model_extra" in "User" shadows an attribute in parent "BaseModel"
  warnings.warn(
/Users/programming/pydantic_work/pydantic/pydantic/_internal/_fields.py:190: UserWarning: Field name "model_fields_set" in "User" shadows an attribute in parent "BaseModel"
  warnings.warn(
"""

I've also enabled support for compiled patterns, though I don't think that's necessary, as all other overrides are methods - which are standard practice to override, and you'll get type checking warnings if signature isn't compatible or b) warnings if something conflicts with the protected namespaces.

@@ -273,7 +273,7 @@ def push(self, config_wrapper: ConfigWrapper | ConfigDict | None):
ser_json_inf_nan='null',
validate_default=False,
validate_return=False,
protected_namespaces=('model_',),
protected_namespaces=('model_validate', 'model_dump'),
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to add model_construct here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, cc @samuelcolvin @dmontagu

Copy link
Contributor Author

@sydney-runkle sydney-runkle Oct 2, 2024

Choose a reason for hiding this comment

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

Following up here - we decided we didn't need to add model_construct - that's not a namespace we anticipate expanding in the same way we do for model_dump_ etc, you can imagine python, json, some_other_format.

@sydney-runkle sydney-runkle merged commit eb9b838 into main Oct 2, 2024
61 checks passed
@sydney-runkle sydney-runkle deleted the protected-ns branch October 2, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-change Used for changes to existing functionality which don't have a better categorization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax default protected_namespaces config setting (remove model_)
4 participants