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

Move aliasing logic from Core into Travertino #3213

Merged
merged 27 commits into from
Mar 11, 2025

Conversation

HalfWhitt
Copy link
Contributor

@HalfWhitt HalfWhitt commented Feb 26, 2025

Fixes #3127 (and is 99% of what's necessary for making Pack a dataclass)

I've not yet added any tests to Travertino. Everything passes Toga's tests (including added checks to verify __contains__), so everything works, at least for what's defined in Pack. Holding off on the lower-level testing at first in case there are significant changes to the implementation.

This adds two classes:

  • aliased_property, which tracks an existing property. This handles padding and the directional aliases.
  • paired_property, which sets up an either-or pairing of two potential source-of-truth properties. This handles alignment / align_items. [Edit: this is now alignment_property, defined in the compatibility section of pack.py.]

Now that all property names, including aliases, are explicitly defined as descriptors, they play nicely with __contains__ and also the dataclass decorator. (I could add that in this PR, but it requires a tweak to the error text matched by a couple of tests, and I didn't want to muddy the waters here.)

Some comments on specific bits inline...

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

@freakboy3742
Copy link
Member

To gauge expectations: We were looking at a Toga release at the end of this week; are you hoping this will be in 0.5.0, or it is for a later release? There's no problem holding off on a release - but if there's no particular reason to hold off, then I won't.

@@ -113,11 +113,24 @@ def layout(self, viewport):

def update(self, **properties):
"""Set multiple styles on the style definition."""
# Some aliases may be valid only in the presence of other property values, and
# this update might be setting those prerequisite properties. So we need to
# defer setting any conditional aliases until last.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could theoretically cause loops, if aliases depend on other aliases. But... there shouldn't ever be a reason to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Or, not loops, but incorrect behavior I suppose.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Un-resolving this — I mistakenly resolved it when I resolved the other two, which were specifically about paired_property. This is still relevant for aliases, as you also noted.

@HalfWhitt
Copy link
Contributor Author

To gauge expectations: We were looking at a Toga release at the end of this week; are you hoping this will be in 0.5.0, or it is for a later release? There's no problem holding off on a release - but if there's no particular reason to hold off, then I won't.

It would be neat to get this and @dataclass into 0.5.0. Though if it needs extensive reworking, I'm not dead-set on that.

@HalfWhitt
Copy link
Contributor Author

Oh, I forgot to mention above: this also fixes "hyphenated-name" in style checks, which I'd never even noticed were broken.

@HalfWhitt HalfWhitt marked this pull request as ready for review February 26, 2025 05:59
@HalfWhitt
Copy link
Contributor Author

Y'know, I keep thinking of draft/ready as whether or not it's potentially (pending changes) ready for merging, but it is explicitly labeled ready for review. I think this is ready for at least a once-over of the design, before I start writing the lower-level tests.

@HalfWhitt HalfWhitt changed the title Move aliasing logic from Toga into Travertino Move aliasing logic from Core into Travertino Feb 26, 2025
@HalfWhitt HalfWhitt mentioned this pull request Feb 26, 2025
4 tasks
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.

There's essentially 2 parts here - aliased properties, and paired properties.

I understand the use case for aliased properties. However, I'm not sure the API you've specified here is sufficient - or, if it is, I'm not understanding how it would extend.

Right now, each of the alignment aliases maps to a single property - but that's a limitation caused by the underlying properties justify_items and align_content not existing. Once those properties exist, each alias will switch how it maps depending on whether the box direction is ROW or COLUMN - so there will be 2 conditions, with a different property mapping. Have I missed something in how this would be used for the future case?

I'm less convinced about paired properties. The only use case we have (that I can see) is alignment, and that's a deprecated property with some inherently complicated mapping to the "new" property because of oversights in how it was specified. I'd hope that in 6-12 months, we're in a position to remove this alias entirely... at which point we shouldn't have any need for a "generic framework for paired properties".

It definitely simplifies the dunder methods on Pack... but I'm not sure those were a maintenance overhead that were causing a problem (bugs with - notation notwithstanding). If introducing something for handling alignment is needed to simplify the overall property handling, I'd be much more inclined to go down the alignment_property path as a purpose-built workaround for the needs of that specific property, rather than an extensible framework that we'll likely delete in a year.

Y'know, I keep thinking of draft/ready as whether or not it's potentially (pending changes) ready for merging, but it is explicitly labeled ready for review. I think this is ready for at least a once-over of the design, before I start writing the lower-level tests.

FWIW, I read "draft" as "I'm tinkering - don't bother looking at this unless you're really interested or concerned", and "non draft, no review requested" as "I'm still working on this, but it's likely getting close to it's final form if you want to be proactive about a review".

@HalfWhitt
Copy link
Contributor Author

Right now, each of the alignment aliases maps to a single property - but that's a limitation caused by the underlying properties justify_items and align_content not existing. Once those properties exist, each alias will switch how it maps depending on whether the box direction is ROW or COLUMN - so there will be 2 conditions, with a different property mapping. Have I missed something in how this would be used for the future case?

I didn't catch that... but yes, I've looked back at the table in #3111 and I see what you mean. I'll work on that.

I'm less convinced about paired properties. The only use case we have (that I can see) is alignment, and that's a deprecated property with some inherently complicated mapping to the "new" property because of oversights in how it was specified. I'd hope that in 6-12 months, we're in a position to remove this alias entirely... at which point we shouldn't have any need for a "generic framework for paired properties".

It definitely simplifies the dunder methods on Pack... but I'm not sure those were a maintenance overhead that were causing a problem (bugs with - notation notwithstanding). If introducing something for handling alignment is needed to simplify the overall property handling, I'd be much more inclined to go down the alignment_property path as a purpose-built workaround for the needs of that specific property, rather than an extensible framework that we'll likely delete in a year.

That makes sense. I certainly hope we won't need to deprecate anything else in such a complicated fashion! We do need some kind of explicit class attribute in order for dataclass to work, and it needs to be a descriptor for in to work without refactoring, but yes I can hard-code it as a custom alignment_property.

FWIW, I read "draft" as "I'm tinkering - don't bother looking at this unless you're really interested or concerned", and "non draft, no review requested" as "I'm still working on this, but it's likely getting close to it's final form if you want to be proactive about a review".

Unless I'm missing something, I don't have the ability to explicitly request a reviewer, unless they've already reviewed it at least once.

@HalfWhitt
Copy link
Contributor Author

@freakboy3742 Does this look more reasonable? aliased_property can now map to various properties depending on the matched condition, and paired_property still exists, but only in Pack's backwards compatibility section to hold shared code between alignment and align_items.

@mhsmith
Copy link
Member

mhsmith commented Feb 28, 2025

Unless I'm missing something, I don't have the ability to explicitly request a reviewer, unless they've already reviewed it at least once.

That's true, you need to be a project member to do that.

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Feb 28, 2025

So... in its current state, the alignment_property in Pack's backwards compatibility section is very close to the paired_property I first proposed; I even kept the approach of deriving the value by iterating through a dictionary of conditions rather than hard-coding the logic in __get__ methods. This may seem unnecessarily generalized, but I went with it because:

  • We need an explicit class attribute; defining things only in __getattribute__ et al doesn't play nicely with @dataclass.
  • Unless we want to refactor or put a custom shim overriding BaseStyle.__contains__, we need that attribute to be a descriptor so it can implement is_set_on (and we need that implementation on both alignment and align_items).
  • Once those are accommodated, it turns out using the dictionary of conditions is shorter than hard-coded __get__s, and easier (for me, anyway) to read. (Compare hard-coded to dictionary-based)

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.

I've only done a quick review pass to get you unblocked, but I think this is all headed in the right direction. Your explanation of why an alignment property is required all makes sense; and while it's definitely a complex beast, I can see how the "meta" definition simplifies some aspects, so it doesn't overly concern me.

The only areas that are giving me pause are ensuring the order of evaluation is correct - i.e., ensuring that the "real" property is defined before the alias, etc, so that effective values and error handling cascade as expected. It's entirely likely you've got it all correct (the fact that this is passing CI definitely suggests this is the case) - but the implementation is just complex enough that it's not inherently obvious (to me, at least) that this is the case - I'll need to spend some quality time with this code during a full review :-)

But for now - I think it's definitely solid enough to fill out the rest of the tests.

@HalfWhitt
Copy link
Contributor Author

This test shouldn't be necessary anymore in core, because we're no longer doing anything that overrides or alters how BaseStyle handles attribute lookup. However, I don't see any harm in leaving well enough alone until this whole section of backwards compatibility eventually gets yanked.

def test_bogus_property_name():
"""Invalid property name in brackets should be an error.
This is tested here along with deprecated property names, because normally it should
be verified by Travertino's tests. It can only be an issue as long as we're
overriding the relevant methods in Pack.
"""
style = Pack()
with pytest.raises(KeyError):
style["bogus"] = 1
with pytest.raises(KeyError):
style["bogus"]
with pytest.raises(KeyError):
del style["bogus"]

@HalfWhitt HalfWhitt requested a review from freakboy3742 March 4, 2025 03:49
@freakboy3742
Copy link
Member

FYI - I've seen this, but I'm currently fighting a monster head cold, so I'm not exactly thinking straight. This will be high on my list once I'm operational again.

@HalfWhitt
Copy link
Contributor Author

FYI - I've seen this, but I'm currently fighting a monster head cold, so I'm not exactly thinking straight. This will be high on my list once I'm operational again.

Oof, feel better soon! (And go back to bed, it's way too early over there!) 🤣

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.

Ok - the flu meds have finally worn off, so I can review this :-)

... and I can hardly find fault at all. Despite the extra classes involved, I'd argue it's much easier to reason about what is going on because it's so well compartmentalised. Between the historical testing, the new tests you've added, and the improved clarity of the code, it all seems reasonably straightforward. I've pushed 2 really minor optimisations, but otherwise, this is awesome work!


def __str__(self):
return "; ".join(
[f"{name} == {value}" for name, value in self.properties.items()]
Copy link
Member

Choose a reason for hiding this comment

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

A really minor optimisation - there's no need to roll out the full list with a comprehension; you can use a generator with join.

Suggested change
[f"{name} == {value}" for name, value in self.properties.items()]
f"{name} == {value}" for name, value in self.properties.items()

Copy link
Contributor Author

@HalfWhitt HalfWhitt Mar 11, 2025

Choose a reason for hiding this comment

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

I was under the impression that, while it's not necessary, it is very slightly faster, since join always builds a list anyway. I am having trouble finding a more recent claim, though — is that no longer the case?

Copy link
Contributor Author

@HalfWhitt HalfWhitt Mar 11, 2025

Choose a reason for hiding this comment

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

I believe this is the equivalent spot in the current code:

https://github.com/python/cpython/blob/6e5b9f3d00658ce07909d78d1a796e95ad5dba1a/Objects/stringlib/join.h#L25

The timeit examples are still in the current 3.13 version of the docs that SO question links to, and I get similar results in 3.13:

$ python -m timeit "'-'.join(str(n) for n in range(100))"
50000 loops, best of 5: 7.24 usec per loop
$ python -m timeit "'-'.join([str(n) for n in range(100)])"
50000 loops, best of 5: 5.79 usec per loop

(Edit: I've edited the question and answer on SO to update the links and quoted code.)

Copy link
Member

Choose a reason for hiding this comment

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

I think the small increase in readability would almost always outweigh the small decrease in speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I don't think I find it any less legible, so I've been in the habit of giving join list comps as a general best practice. But I have no objection to the opposite as a style guide. It's not like the performance difference is likely to ever actually matter.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting... TIL that this isn't as straightforward as I thought. Agreed that in this case, the performance implication isn't really a consideration; as for readability... it's fairly marginal either way. Regardless, the code has landed now, so it's probably not worth updating.

@freakboy3742 freakboy3742 merged commit 678cdf6 into beeware:main Mar 11, 2025
48 checks passed
@HalfWhitt
Copy link
Contributor Author

Ok - the flu meds have finally worn off, so I can review this :-)

Glad you're feeling better!

... and I can hardly find fault at all.

Apparently the lesson here is to wait a week before submitting a PR for review, so I obsessively check back over it enough times to notice all the problems! 🤣

@HalfWhitt HalfWhitt deleted the travertino-aliases branch March 12, 2025 02:52
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.

Align/Justify aliases don't work with __contains__
3 participants