-
Notifications
You must be signed in to change notification settings - Fork 23
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
Allow simple container literals as default values #358
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This change has no effect on typeshed. 🤖🎉 |
field226: tuple[str | tuple[str, ...], ...] = ("foo", ("foo", "bar")) # Y015 Only simple default values are allowed for assignments | ||
field227: dict[str, object] = {"foo": {"foo": "bar"}} # Y015 Only simple default values are allowed for assignments |
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 think these are decent examples, why we shouldn't forbid nesting. It's extra code and might occasionally be useful. If it becomes too complex, a manual reviewer can always point this out.
That said, this is not a show stopper.
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 thought you might say that :)
I guess my worry is that we've added most of our defaults up till now using stubdefaulter
, which can result in a huge PR that we can't really review manually. I wouldn't really want a default such as {(("foo", "bar"), (1, 2): {("foo", "bar"): {1, 2, 3}}}
added automatically by stubdefaulter in some bizarre third-party library to slip through human review by being bundled in a huge PR. But I guess that's pretty unlikely; I don't feel too strongly about disallowing nesting.
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.
LGTM, we can loosen the warnings later when all existing stubs have been stubdefaulted.
Resolves #357