Add font shorthand property#3631
Conversation
| VALUE2, VALUE3, VALUE4, initial=VALUE2 | ||
| ) | ||
|
|
||
| composite_no_optional: tuple[str] = composite_property( |
There was a problem hiding this comment.
This incorrect type hint is basically a handwave. I can't imagine it's worth actually laying out all the various possible signatures for a test class, is it, as long there's some hint to trigger dataclass?
I notice I also didn't do the various tuple type hints right on the directional property and its aliases...
There was a problem hiding this comment.
I agree - exhaustive hinting here will be exhausting.
HalfWhitt
force-pushed
the
font_shorthand
branch
from
e5cef24 to
bf13813
Compare
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
This looks great - and thanks to all the refactoring that precedes it, the implementation is incredibly straightforward. Nice work.
A couple of really cosmetic things flagged inline, but otherwise, this looks almost ready to land.
| root = ExampleNode( | ||
| "app", | ||
| style=Pack( | ||
| @pytest.mark.parametrize("use_shorthand", [True, False]) |
There was a problem hiding this comment.
As a stylistic thing - my preference here would be to factor out the arguments to Pack as the parameterised argument - that way, we're testing multiple ways to construct the same thing, rather than two independent constructors that happen to have the same result.
| f"Composite property '{self.name}' must be set with at least " | ||
| f"{self.min_num} and no more than {self.max_num} values." |
There was a problem hiding this comment.
As an error, this is 100% correct... but not especially helpful to an end user. It might be more helpful to elaborate the specific attributes that must be provided (.e.g, "Composite property 'font' must provide a value for 'font_size' and 'font_family', and may also provide a value for 'font_style', 'font_variant' and 'font_weight'.)
There was a problem hiding this comment.
Oh right, people are actually going to use this, aren't they?
Testing this out also alerted me to something else I'd missed — we need to make sure whatever's being assigned is a non-string sequence.
| VALUE2, VALUE3, VALUE4, initial=VALUE2 | ||
| ) | ||
|
|
||
| composite_no_optional: tuple[str] = composite_property( |
There was a problem hiding this comment.
I agree - exhaustive hinting here will be exhausting.
| assert_composite(style, (None, VALUE2, VALUE2, VALUE3)) | ||
| assert "different_values_prop" not in style | ||
| assert "explicit_none" not in style |
There was a problem hiding this comment.
This test looks completely correct, but could do with some explanatory notes so it's clear what it's testing.
|
Do you have any thoughts on potential future string-parsing? |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks good (and good catch on the 'string as sequence' thing).
As for potential future string parsing... I'm not sure exactly what input you're looking for there. Yes, I think string parsing would be desirable, if only in the context of "we should be able to provide stylesheets as external files"; to that end, anything that currently requires a tuple as input should be possible to provide as a string that will be parsed into a tuple.
However, I can't say I've given the topic much thought beyond that.
|
Well, I went into it up in the initial post, but basically I'm thinking about the tradeoffs of writing our own parser vs. using some variety of grammar-defining library. Dealing with things like nested and escaped quotes (and single and double quotes) gets hairy pretty fast. |
2 participants
Adds the
fontshorthand property to Pack, and thecomposite_propertyclass in Travertino that implements it.The
composite_propertyimplementation is largely verbatim from the old PR on Travertino. The only significant change is that I've taken out any facility for string-parsing, which I wanted to ask about.I have a reasonably competent implementation already for how to parse the
fontproperty from a string, but it definitely doesn't handle all the potential edge cases around mixed quotation marks and escaped characters and whatnot. I'd like to accommodate the feature, either in this PR or a subsequent one, because being able to specifyfontall as one string would certainly be convenient, but string-parsing is complicated. Presumably, at some point, Colosseum will need to be able to parse entire CSS files, at which point I imagine a dependency on something like pyparsing would probably be justified, but I'm not sure if it is for just this one property. So I'd appreciate some input on these potential avenues:Even in the absence of string-parsing, though, assigning with a tuple works perfectly well.
P.S. Also adds
.ruff_cache/to .gitignore.PR Checklist: