-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Make json and yaml available in templates #28930
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits
Also we should add info into the Templates reference documentation. Otherwise how users would know how to use this macro?
airflow/macros/__init__.py
Outdated
|
||
def deserialize(data: str) -> dict[Any, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deserialize
it is a bit broad name which could mean everything and might confuse users.
172cfdc
to
a2fcdc1
Compare
Sounds good! I have integrated the requested changes in my latest push, including changing the name of the macro from However, in terms of the documentation, I think it will already automatically be added to the templates reference. This is because this macro is a macro function, specified in airflow/macros along with |
I’d prefer adding two separate macros, one for only JSON and another for YAML support. |
a2fcdc1
to
c54c7af
Compare
c54c7af
to
849e72e
Compare
Sounds good! I have created those two separate macros and have rebased. Let me know if any other changes are needed. |
Hmm, looking at other things in the module, I wonder if we should simply import |
Yeah. It woudl be useful to add json/yaml generic imports for many other use cases. The only potential problem is safety of those, but I think those two shoudl be rather safe to add. |
That makes sense. Would it still make sense to keep the tests on json and yaml, or should those be removed too? |
Keeping two tests is fine. |
8cdae66
to
8b38ee4
Compare
8b38ee4
to
a195148
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up, those noqa
comments should be removed and replaced by an explicit whitelist.
Hi everyone, it seems like this PR is approved. I was wondering if there is anything else that should be updated or changed? If so, let me know. |
Hi everyone, it seems like this PR has been approved and passes all the tests. I was wondering if there is anything else that should be updated or changed? If so, let me know. |
Hi everyone, I was wondering, since the changes on this branch have been approved and it seems to be passing all the tests, if the branch could be merged into main? Or if there is anything else that needs to be updated or changed, let me know. |
closes: #27079
It was suggested that a macro be added that allows for the deserialization of json or yaml strings (since yaml is a superset of jsons, I just deserialized using yaml).
I also added tests for this macro in
tests/macro
.I would also be open to adding this functionality to the system tests if that is needed.