-
-
Notifications
You must be signed in to change notification settings - Fork 19
Declare properties as class attributes rather than via methods #141
Conversation
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 yak may contain multitudes, but it's fur is oh so luxurious :-)
The actual data class implementation itself looks amazing - I've flagged a couple of minor things, but on the whole it looks great.
The backwards compatibility considerations all look sound - the only concern I had was around the kw_only
handling on Py 3.8/3.9, and how we'll accomodate that in Toga - but I'm guessing the answer is essentially the same as what you've got in the test case.
The biggest set of requested changes will be in the test case - as I flagged inline, we're trying to move to pytest. We don't need to migrate all the existing tests as part of this PR; but if we're adding a big chunk of new tests, then it doesn't make much sense to add them in a format we know (or hope) is going to be replaced.
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
…d used sys.version for 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.
Nice work on de-duplicating the tests; I'm not concerned about having a bunch of "deprecation only" tests in the files.
I've flagged a couple of issues inline; two of which are big-ish but should be resolvable (missing apply() checks, and the change in API path on directional_property
)
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 looks great! Thanks for the cleanups.
Fixes #11
I have shaved what may or may not be an inadvisable quantity of yak.
As saroad2 wisely observed in #12, cleaning up the syntax and getting auto-completion is a lot more feasible now that all target versions of Python support dataclasses!
An example is worth a thousand words, so basically what was this:
can now become this:
There are, inevitably, some caveats. I've accounted for two kinds of backwards compatibility:
kw_only
argument was only added in 3.10... and any code using existing versions of Toga won't be using@dataclass
at all. So there's an essentially unchanged__init__
on the base class as a fallback, and 3.8/3.9 setinit=False
instead ofkw_only
. This runtime jiggery-pokery doesn't seem to interfere with PyCharm's ability to autocomplete the parameter names.Other caveats:
I also did some general old-code sprucing up: defaultdicts, f-strings, comprehensions, that sort of thing.
PR Checklist: