Skip to content
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 'factory' support for the Any trait type #1557

Merged
merged 6 commits into from
Sep 30, 2021
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Sep 29, 2021

This PR adds factory, args and kw keyword-only arguments to the Any trait type initializer, allowing a default value for an Any trait to be specified via a factory, in much the same way as this already works for the Instance trait type.

The main goal is to provide a clean migration path for existing code using Any({}) or Any([]). With Traits 6.3, such code will run into the deprecation introduced in #1532, and can now use Any(factory=dict) or Any(factory=list) as a suitable replacement that won't warn.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

@mdickinson
Copy link
Member Author

Looking into the stubs-related test failure ...

@mdickinson
Copy link
Member Author

I think the mypy complaint is legitimate here. I've updated the test accordingly.

@mdickinson
Copy link
Member Author

Whoops; the user manual does need updating here. Now done.

@@ -278,7 +278,9 @@ the table.
+------------------+----------------------------------------------------------+
| Name | Callable Signature |
+==================+==========================================================+
| Any | Any( [*value* = None, \*\*\ *metadata*] ) |
Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected the name of the initial parameter from value to default_value (though I expect few people will be passing that parameter by name).



class Subclass(Superclass):
x = Instance(Foo)
x = Instance(Foo) # E: assignment
Copy link
Member Author

Choose a reason for hiding this comment

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

This line didn't raise before this PR, and does now; I believe that this was a bug in the test. mypy infers from Superclass that the attribute x should have type Any, and complains about the non-matching assignment in the subclass, which seems like a legitimate complaint.

@mdickinson mdickinson added this to the 6.3.0 release milestone Sep 29, 2021
*,
factory=None,
args=(),
kw={},
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is considered an antipattern (compared to using kw=None and then explicitly checking for None in the body of the function). No, I don't want to change it unless people feel really strongly about it. A default of {} makes the code, documentation and typing stubs simpler and clearer, and the usual concern about mutable default values doesn't apply here, because there's no plausible way that accidental mutation of this default value could take place.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM with one small comment about documentation

docs/source/traits_user_manual/defining.rst Outdated Show resolved Hide resolved
mdickinson and others added 2 commits September 30, 2021 08:08
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
@mdickinson mdickinson merged commit 95632b1 into main Sep 30, 2021
@mdickinson mdickinson deleted the feature/any-factory branch September 30, 2021 07:23
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.

None yet

2 participants