-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
added a key_trait argument to Dict #306
Conversation
I do think that constructor keywords are confusing as they are. I would propose to change |
|
||
def __init__(self, trait=None, traits=None, default_value=Undefined, | ||
def __init__(self, trait=None, key_trait=None, traits=None, default_value=Undefined, |
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.
To avoid backward-incompatibility with positional args, can you add key_trait to the end?
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.
To be honest, I think this schema feature accessible via the traits
argument doesn't belong into Dict
at all. I'd rather not have it block access to key_trait
. What about a compromise: Keep key_trait
before traits
and when an instance of dict
(the python class, not the trait) is encountered in key_trait
, raise a warning and map it to traits
?
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.
My preference would be for all of these after the first to be keyword-only arguments, but Python 2 doesn't make that very easy other than **kwargs
. I would add it to the end and treat it as if it were keyword-only.
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.
To me the keys of a dict are at least as important as the values if not more. I have the feeling everyone here seems to implicitly assume unicode keys for their dicts, but that is not validated in the current implementation. I'd put the key trait even before the value trait if not for backwards compatibility.
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 concerned more with clarity than priority. I don't view the order as being significant (to me, there is only one positional arg for any TraitType, but Python 2 doesn't make this easy). If you'd like to make value_trait
and key_trait
, that would be fine by me, as long as it's done in a backward-compatible way (i.e. positional args retain their order). I wouldn't expect anyone to pass key_trait positionally ever, and I'm AOK encouraging people to pass value_trait by keyword-arg, as well, since that would be clearest.
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 don't see how there is a general one positional arg that would have a well defined meaning for all TraitType's, so the positional args could equally well be trait specific. But then, I guess backwards compatibility shouldn't be broken lightly, so it's anyway too late. I'll shut up now and will just fix backwards compatibility.
If you want to add a value_trait arg and deprecate |
…cated previous keywords in favour of more descriptive ones
Thanks! |
As a followup to #306, this updates tests and docs to use the non-deprecated value_trait and per_key_traits arguments.
Fixes #304