Skip to content

Improve typehint #77

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

Closed
wants to merge 6 commits into from
Closed

Improve typehint #77

wants to merge 6 commits into from

Conversation

heckad
Copy link

@heckad heckad commented Apr 20, 2020

No description provided.



@typing.overload
def class_schema(clazz: type, base_schema: T_SchemaType) -> T_SchemaType:
Copy link
Owner

Choose a reason for hiding this comment

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

Is this annotation correct though ? The object returned by this function is a class that inherits from T_SchemaType, not T_SchemaType itself...

Copy link
Author

Choose a reason for hiding this comment

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

This function should return the same type that sanded throw base_schema. If base_schema is None then return default type.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I understand the problem with the two different overloads for this function. However, I don't think the type annotation you added is correct. The type of base_schema and the output type of the function are not the same.

@lovasoa
Copy link
Owner

lovasoa commented Apr 20, 2020

Can you give context about why this is needed ?

@heckad
Copy link
Author

heckad commented Apr 20, 2020

import typing
from dataclasses import dataclass
from typing import Union

import marshmallow_dataclass
from marshmallow import Schema as BaseSchema
from marshmallow import SchemaOpts as BaseSchemaOpts
from marshmallow import types


class SchemaOpts(BaseSchemaOpts):
    def __init__(self, meta, **kwargs):
        super().__init__(meta, **kwargs)
        self.required_fields = getattr(meta, "required_fields", set())


class MySchema(BaseSchema):
    OPTIONS_CLASS = SchemaOpts

    def __init__(self, *, required_fields=set(), **kwargs):
        self.required_fields = required_fields or self.opts.required_fields
        super().__init__(**kwargs)

    def on_bind_field(self, field_name, field_obj):
        super().on_bind_field(field_name, field_obj)
        if field_name in self.required_fields:
            field_obj.required = True

    def make_data_class(self, data, **_):
        super().make_data_class(data, **_)

    def loads(
        self,
        json_data: Union[str, bytes, bytearray],
        *,
        many: bool = None,
        partial: typing.Union[bool, types.StrSequenceOrSet] = None,
        unknown: str = None,
        **kwargs
    ):
        return super().loads(
            json_data,  # type: ignore
            many=many, partial=partial, unknown=unknown, **kwargs)


@dataclass
class A:
    a: int


schema = marshmallow_dataclass.class_schema(A, base_schema=MySchema)
schema().loads(b'')

Mypy output

test.py:52: error: Argument 1 to "loads" of "Schema" has incompatible type "bytes"; expected "str"
Found 1 error in 1 file (checked 1 source file)

Thats because mypy thing that schema have type Schema instead of MySchema

@lovasoa
Copy link
Owner

lovasoa commented Apr 20, 2020

Also, can you fix the tests ? You can run pre-commit to check your work before pushing it.

@heckad
Copy link
Author

heckad commented Apr 20, 2020

Done

@heckad
Copy link
Author

heckad commented Apr 21, 2020

Thanks a lot!

@lovasoa
Copy link
Owner

lovasoa commented Apr 21, 2020

@heckad : I'm trying to write tests for this and it doesn't seem to be supported by mypy, though...

error: Unsupported type Type[T_Schema?]

python/mypy#4300

@lovasoa
Copy link
Owner

lovasoa commented Apr 21, 2020

This makes me think we should probably wait for python/mypy#4300 (opened in 2017) to be fixed before merging this PR. The Unsupported type Type[T_Schema?] cannot be silenced, so I fear merging this may cause a lot of trouble to users...

@lovasoa
Copy link
Owner

lovasoa commented Apr 21, 2022

type hints should have been fixed

@lovasoa lovasoa closed this Apr 21, 2022
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