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
Dataclass extra #3393
Dataclass extra #3393
Conversation
d9bc078
to
5074f94
Compare
please review |
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 reopened the original issue, no idea why I closed it, must have been a mistake.
I'm also a bit confused about why tests weren't run, hopefully if you commit again they'll be triggered.
a: str | ||
b: str | ||
|
||
Foo(**{"a": "bar", "b": "foo", "c": 1}) |
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.
- single quotes
Foo(a='bar'...)
would be cleaner- you need to add
assert not hasattr(foo, 'c')
- you need to add another test for
Extra.allow
k: v for k, v in kwargs.items() if k in get_type_hints(type(self)) | ||
}) | ||
|
||
cls.__original_init__ = cls.__init__ |
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.
Looks like this is never called. We need a test for this.
}) | ||
|
||
cls.__original_init__ = cls.__init__ | ||
cls.__init__ = allow_extra_init |
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.
@PrettyWood I remember we had lots of problems with overriding __init__
at some points.
Do you think this will break anything?
What about conflicts with #2557, will it just be code conflicts, or does this approach need to be rethought?
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'm sorry @DetachHead but I felt like it was way easier to add it in 30b58f3 rather than handling conflicts :/
@samuelcolvin IMO if the commit above is good we can close this PR
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.
@PrettyWood looks good. though i can't find the branch that commit is on? is there a PR for it?
please update. |
closing since #2557 was merged |
Change Summary
forked from @PrettyWood's change which allows using the
extra
config option for dataclasses.see #986 (comment)
Related issue number
fix #986
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)