Skip to content
This repository has been archived by the owner on Jan 25, 2025. It is now read-only.

Declare properties as class attributes rather than via methods #141

Merged
merged 26 commits into from
Mar 5, 2024

Conversation

HalfWhitt
Copy link
Contributor

@HalfWhitt HalfWhitt commented Feb 27, 2024

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:

class Pack(BaseStyle):
    ...

Pack.validated_property("width", choices=SIZE_CHOICES, initial=NONE)
Pack.validated_property("height", choices=SIZE_CHOICES, initial=NONE)
Pack.validated_property("flex", choices=FLEX_CHOICES, initial=0)

Pack.validated_property("padding_top", choices=PADDING_CHOICES, initial=0)
Pack.validated_property("padding_right", choices=PADDING_CHOICES, initial=0)
Pack.validated_property("padding_bottom", choices=PADDING_CHOICES, initial=0)
Pack.validated_property("padding_left", choices=PADDING_CHOICES, initial=0)
Pack.directional_property("padding%s")

can now become this:

@dataclass(kw_only=True)
class Pack(BaseStyle):
    width: str | int = validated_property(choices=SIZE_CHOICES, initial=NONE)
    height: str | int = validated_property(choices=SIZE_CHOICES, initial=NONE)
    flex: float = validated_property(choices=FLEX_CHOICES, initial=0)

    padding: tuple[int] = directional_property(
        "padding{}", choices=PADDING_CHOICES, initial=0
    )
    padding_top: int
    padding_right: int
    padding_bottom: int
    padding_left: int
    
    ...

There are, inevitably, some caveats. I've accounted for two kinds of backwards compatibility:

  1. Use of the existing/previous API. The class methods are still there, they just hook into the newer API to get the same thing done.
  2. The 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 set init=False instead of kw_only. This runtime jiggery-pokery doesn't seem to interfere with PyCharm's ability to autocomplete the parameter names.

Other caveats:

  • The directional subproperties probably could be added entirely dynamically, even to the dataclass's init fields, but since one of the benefits of this is auto-completion and PyCharm et al would almost certainly not follow that, I didn't bother going down that road.
  • Travertino passes all its existing tests, as well as Toga's. (And also passes a version of Toga updated to use the newer syntax.) As a quick-and-dirty way to simultaneously verify both APIs, I copied over both relevant test files in their entirety and updated them. (I also factored the apply-mocking out into a decorator, but that shouldn't be a functional change, it's just convenient.) Obviously two entire copies of all those tests, in separate files, isn't the cleanest or most maintainable testing structure going forward. But since I imagine any testing overhaul should probably involve switching to pytest anyway, I figured any elaborate restructuring would be premature for this PR.

I also did some general old-code sprucing up: defaultdicts, f-strings, comprehensions, that sort of thing.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a 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.

Copy link
Member

@freakboy3742 freakboy3742 left a 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)

@HalfWhitt HalfWhitt requested a review from freakboy3742 March 3, 2024 00:11
Copy link
Member

@freakboy3742 freakboy3742 left a 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.

@freakboy3742 freakboy3742 merged commit 6adad4c into beeware:main Mar 5, 2024
8 checks passed
@HalfWhitt HalfWhitt deleted the style-dataclass branch March 6, 2024 00:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Change BaseStyle from class to decorator
2 participants