Registering custom YAML tags #260
Conversation
bmispelon
force-pushed
the
custom-yaml-loader
branch
2 times, most recently
from
45cadff to
f502b7b
Compare
bmispelon
force-pushed
the
custom-yaml-loader
branch
from
f502b7b to
24db917
Compare
thibaudcolas
added
enhancement
| """ | ||
| # PyYAML doesn't provide an inverse operation for add_constructor(), so | ||
| # we need to do it manually. | ||
| del PatternLibraryLoader.yaml_constructors[f"!{name}"] |
There was a problem hiding this comment.
I'm not familiar with the pyyaml internals, but won't this mutate the yaml_constructors list of the parent class, FullLoader?
There was a problem hiding this comment.
Great question! I believe it won't, because of the copy() there: https://github.com/yaml/pyyaml/blob/69c141adcf805c5ebdc9ba519927642ee5c7f639/lib/yaml/constructor.py#L162
But I should probably add a test for that behavior to be certain.
There was a problem hiding this comment.
Ohhh that makes sense. Thanks for checking.
There was a problem hiding this comment.
Nice! No need to create an issue, though if you have the chance it’s always appreciated to check if there are related issues where this is worth mentioning / that can be triaged with this change in mind (#155 ?)
- I know nothing about YAML tags and PyYAML. Could you either find someone who does to review this, or provide guidance on which parts of the code need attention?
- I think it would be nice to have an additional example in the docs for this that isn’t Wagtail-related.
- Could you add examples of this in the pattern library demo, so it’s easier to manually test? Those can be based on your test cases, or something more advanced that better demonstrates the benefits.
- As part of this, could you help us triage Can't use Python object YAML tags #155? Seems like that issue would still be relevant, possibly, though with your changes we might be ok to fully treat "no support for arbitrary Python object tags" as a feature and close that issue.
| if "main_image" in context["page"]: | ||
| context["main_image"] = Image.objects.all().order("?").first() | ||
| context["main_image"] = Image.objects.all().order_by("?").first() |
|
|
||
| ## Extending the YAML syntax | ||
|
|
||
| You can also take advantage of YAML's local tags in order to insert full-fledged Python objects into your mocked contexts. |
There was a problem hiding this comment.
| You can also take advantage of YAML's local tags in order to insert full-fledged Python objects into your mocked contexts. | |
| You can also take advantage of [YAML's local tags](https://yaml.org/spec/1.2.2/#tags) in order to insert full-fledged Python objects into your mocked contexts. |
I think an external explainer link would be good here? Have added one but it’s a bit dry. If you have a better one please add it?
| from wagtail.images import get_image_model | ||
|
|
||
| @register_yaml_tag | ||
| def testimage(): |
There was a problem hiding this comment.
| def testimage(): | |
| def test_image(): |
testimage and concatenatedwordslikethis are problematic for accessibility – not very readable, don’t work well with text-to-speech (screen readers). So let’s add an underscore.
Also I think this will be a lot of people’s first contact with YAML tags, so well worth it to polish this example more if there are ways? Though "test image" is a reasonable start.
| image: !testimage | ||
| - title: Second item | ||
| image: !testimage | ||
| ``` |
There was a problem hiding this comment.
GitHub’s syntax highlighting seems to fail from here onwards. Not a dealbreaker but just wanted to check if we should mention compatibility of local tags in YAML with different IDEs / linters? For example some projects will use Prettier’s auto-formatting for YAML, not sure if local tags in there are a compatibility problem that warrants mentioning.
| def get_random_image(): | ||
| ... |
There was a problem hiding this comment.
Is this not missing the name=?
|
|
||
| ### Passing arguments to custom tags | ||
|
|
||
| It's possible to create custom tags that take arguments. |
There was a problem hiding this comment.
Say more about why this is useful? Something like:
| It's possible to create custom tags that take arguments. | |
| It's possible to create custom tags that take arguments, to get more control over how the mock data is generated. |
Even better to have a more complete example
| return constructor | ||
|
|
||
|
|
||
| def register_yaml_tag(fn=None, name=None): |
There was a problem hiding this comment.
The package’s public API uses type hints, so this will need adding here. Same for unregister_yaml_tag. It’s fine for the implementation and tests to not be typed.
I could review it. It's easier since I was involved with an internal task, that we decided to push upstream, so I have the context. |
Description
I recently learned about registering/using custom YAML tags and they pair well with Django Pattern Library. Once thing though is that custom tags are currently registered globally and could therefore theoretically have unwanted side effect on the application if it uses yaml too.
To solve this, I've split this PR into two commits:
yaml.FullLoadersubclass and uses that when loading a context. This way anyone who wants to register a custom tag can do so on the pattern library's custom loader and not on the global one.There's also a bonus commit that fixes a typo in the code examples that I found by accident while writing my own documentation.
Let me know if I should also create an issue to track this, I'd be happy to!
Checklist