PEP 750: Updates after community feedback #4124
Conversation
Next round of updates to PEP 750
hugovk
changed the title
There was a problem hiding this comment.
I was going to do a full review of this PR but ran out of time. Here are a few comments I have. In any case, I offer to just merge this PR, so readers of the thread won't have to wonder if their changes are accepted. You can always merge another PR -- there's no particular process to be followed here.
| @@ -138,11 +144,23 @@ Template strings evaluate to an instance of a new type, ``templatelib.Template`` | |||
| .. code-block:: python | |||
|
|
|||
| class Template: | |||
| args: Sequence[str | Interpolation] | |||
| args: tuple[str | Interpolation, ...] | |||
There was a problem hiding this comment.
This makes args appear writable. Maybe it should also be a property? You can always use __args for the real storage.
There was a problem hiding this comment.
(Will address when we decide the final layout for Template, since args may be going away.)
|
|
||
| The ``Template.strings`` and ``Template.interpolations`` properties provide | ||
| convenient access to the ``str`` and ``Interpolation`` instances | ||
| found in ``Template.args``. |
There was a problem hiding this comment.
Why not add that they return args[0::2] and args[1::2]?
There was a problem hiding this comment.
(Will address when we decide the final layout for Template, since args may be going away.)
| An ``Interpolation`` instance is hashable if its ``value`` attribute | ||
| is also hashable. Likewise, a ``Template`` instance is hashable if | ||
| all of its ``Interpolation`` instances are hashable. In all other cases, | ||
| ``TypeError`` is raised. |
There was a problem hiding this comment.
I kinda see why you're doing it this way, but it means that a hypothetical caching HTML template processor will have a hard time caching templates reliably (e.g. if a dict of attributes is given as in the example, the template can't be cached, at least its natural hash and eq won't let it). And in such use cases presumably the values in the cache are ignored -- when a new template is processed, its values are combined with the cached processed template.
There was a problem hiding this comment.
My expectation is that the Template.strings tuple will be the common cache key in these scenarios.
There was a problem hiding this comment.
So then I would consider dropping hash and eq.
| No ``Template.__str__()`` Implementation | ||
| ---------------------------------------- | ||
|
|
||
| The ``Template`` type does not provide a specialized ``__str__()`` implementation; | ||
| it inherits the default implementation from the ``object`` class. | ||
|
|
||
| This is because ``Template`` instances are intended to be used by template processing | ||
| code, which may return a string or any other type. There is no canonical way to | ||
| convert a Template to a string. |
There was a problem hiding this comment.
Is there a repr()? It should return the entire template literal (including t" and ") as a string, preserving the text between the {} -- possibly normalizing cases where the template doesn't preserve the exact text inside those.
There was a problem hiding this comment.
Ah, good question. I don't see why not to do this, or something like it. @lysnikolaou are there any implementation issues that would come up?
There was a problem hiding this comment.
IIRC if there is a repr but no str, str falls back to the repr instead of inheriting from object.
I can't believe I didn't catch this in any of my zillion previous re-reads. Sheesh!
Updates include:
Template, includingstrings,interpolations, andvalues.Template.__hash__()Template.argsexplicitly be atupleThanks to @lysnikolaou @koxudaxi @pauleveritt @jimbaker and everyone in recent discussion threads for all the useful feedback.
I opened a new Discourse thread for this updated PR.
📚 Documentation preview 📚: https://pep-previews--4124.org.readthedocs.build/