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

Remove only, exclude, many, and unknown parameters of Nested and require callables to return Schema instances #2794

Open
sloria opened this issue Jan 19, 2025 · 1 comment

Comments

@sloria
Copy link
Member

sloria commented Jan 19, 2025

Proposing the following 2 changes to fields.Nested:

(1) drop support for passing callables that return schema classes and dicts (e.g. lambda: CSchema) and only support callables that return instances (lambda: CSchema()).

Before:

        nested: (
            Schema
            | SchemaMeta
            | str
            | dict[str, Field]
            | typing.Callable[[], Schema | SchemaMeta | dict[str, Field]]
        ),

After:

        nested: (
            Schema | SchemaMeta | str | dict[str, Field] | typing.Callable[[], Schema]
        ),

And (2) deprecate the only, exclude, many, and unknown parameters of Nested.

Final end state of fields.Nested would be

    def __init__(
        self,
        nested: (
            Schema | SchemaMeta | str | dict[str, Field] | typing.Callable[[], Schema]
        ),
        **kwargs: Unpack[_BaseFieldKwargs],
    ):

Why?

It would vastly both the usage and the internal implementation of Nested. This eliminates a class of bugs where the nested argument conflicts with only, exclude, many, and unknown.

See #2165 for an example of this.

@lafrech
Copy link
Member

lafrech commented Jan 26, 2025

I agree it is strange to accept an instance and modifiers applying to that instance. I don't like modifying instances. This is something we may want to prevent (#1281).

But it may be useful for nested schemas passed by name as string, or by class. (I don't see the point of accepting a class, we could require an instance, but I see the point of accepting a schema name as string, which ultimately falls in the same case as a class).

Regarding schemas passed as callable, it may be surprising that only instances are accepted, but if it makes our job easier, I don't mind dropping the class case and require an instance. Doing the instantiation in the callable is not too much to ask. (We never accepted a callable returning a schema name as string, anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants