-
Notifications
You must be signed in to change notification settings - Fork 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
add slots, match_args, kw_only to dataclass func #252
base: master
Are you sure you want to change the base?
Conversation
add slots, match_args, kw_only to dataclass func
args = { | ||
"match_args": { | ||
"error_message": "'match_args' argument is only available for python >= 3.10", | ||
"default_value": True, | ||
}, | ||
"kw_only": { | ||
"error_message": "'kw_only' argument is only available for python >= 3.10", | ||
"default_value": False, | ||
}, | ||
"slots": { | ||
"error_message": "'slots' argument is only available for python >= 3.10", | ||
"default_value": False, | ||
}, | ||
} | ||
|
||
for arg, arg_info in args.items(): | ||
if locals()[arg] is not arg_info["default_value"]: | ||
raise ValueError(arg_info["error_message"]) | ||
|
||
# dataclass's typing doesn't expect it to be called as a function, so ignore type check | ||
dc = dataclasses.dataclass( # type: ignore | ||
_cls, repr=repr, eq=eq, order=order, unsafe_hash=unsafe_hash, frozen=frozen | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this ? Can't we just forward the arguments we got to the base dataclass implementation, and let it error if it wants to ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. For the implementation can we not just do
def dataclass(
__cls: Optional[Type[_U]] = None,
*,
base_schema: Optional[Type[marshmallow.Schema]] = None,
cls_frame: Optional[types.FrameType] = None,
**kwargs: bool,
) -> Union[Type[_U], Callable[[Type[_U]], Type[_U]]]:
dc = dataclasses.dataclass(**kwargs)
if not cls_frame:
current_frame = inspect.currentframe()
if current_frame:
cls_frame = current_frame.f_back
# Per https://docs.python.org/3/library/inspect.html#the-interpreter-stack
del current_frame
def decorate(cls: Type[_U]) -> Type[_U]:
return add_schema(dc(cls), base_schema, cls_frame=cls_frame)
if _cls is None:
return decorate
return decorate(cls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the type signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple of comments here.
Also, see my review of #205 for more (albeit similar) comments.
https://github.com/lovasoa/marshmallow_dataclass/pull/205/files
@@ -117,6 +120,9 @@ def dataclass( | |||
order: bool = False, | |||
unsafe_hash: bool = False, | |||
frozen: bool = False, | |||
match_args: bool = True, | |||
kw_only: bool = False, | |||
slots: bool = False, | |||
base_schema: Optional[Type[marshmallow.Schema]] = None, | |||
cls_frame: Optional[types.FrameType] = None, | |||
) -> Callable[[Type[_U]], Type[_U]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel these overloads should be conditional on python version. I.e. The prototypes should not declare unsupported options.
Since (I think) we want marshmallow_dataclass.dataclass
to have the same signature as dataclasses.dataclass
, we should probably just copy typeshed's interface definition for dataclass
.
args = { | ||
"match_args": { | ||
"error_message": "'match_args' argument is only available for python >= 3.10", | ||
"default_value": True, | ||
}, | ||
"kw_only": { | ||
"error_message": "'kw_only' argument is only available for python >= 3.10", | ||
"default_value": False, | ||
}, | ||
"slots": { | ||
"error_message": "'slots' argument is only available for python >= 3.10", | ||
"default_value": False, | ||
}, | ||
} | ||
|
||
for arg, arg_info in args.items(): | ||
if locals()[arg] is not arg_info["default_value"]: | ||
raise ValueError(arg_info["error_message"]) | ||
|
||
# dataclass's typing doesn't expect it to be called as a function, so ignore type check | ||
dc = dataclasses.dataclass( # type: ignore | ||
_cls, repr=repr, eq=eq, order=order, unsafe_hash=unsafe_hash, frozen=frozen | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. For the implementation can we not just do
def dataclass(
__cls: Optional[Type[_U]] = None,
*,
base_schema: Optional[Type[marshmallow.Schema]] = None,
cls_frame: Optional[types.FrameType] = None,
**kwargs: bool,
) -> Union[Type[_U], Callable[[Type[_U]], Type[_U]]]:
dc = dataclasses.dataclass(**kwargs)
if not cls_frame:
current_frame = inspect.currentframe()
if current_frame:
cls_frame = current_frame.f_back
# Per https://docs.python.org/3/library/inspect.html#the-interpreter-stack
del current_frame
def decorate(cls: Type[_U]) -> Type[_U]:
return add_schema(dc(cls), base_schema, cls_frame=cls_frame)
if _cls is None:
return decorate
return decorate(cls)
I tried to address some of the comments here in another PR: |
This is similar to an outstanding PR for two reasons: