fix: omit also jinja functions #37
Conversation
|
Hmm, I think in conda-smithy and friends they have some "shim" functions for this. Do you think we should implement the necessary jinja logic? |
what do you mean by shims? |
| "split": split_filter, | ||
| } | ||
| ) | ||
| return env |
There was a problem hiding this comment.
Do we also need to add some stub functions for env.get and env.get_default?
There was a problem hiding this comment.
All the more reason tho have an actual recipe that contains some of these corner cases.
There was a problem hiding this comment.
that's true , added mamba_recipe as a test one
| def version_to_build_string(some_string: str) -> str: | ||
| """Converts some version by removing the . character and returning only the first two elements of the version)""" | ||
| # We first split the string by whitespace and take the first part | ||
| split = some_string.split()[0] if some_string.split() else some_string | ||
| # We then split the string by . and take the first two parts | ||
| parts = split.split(".") | ||
| major = parts[0] if len(parts) > 0 else "" | ||
| minor = parts[1] if len(parts) > 1 else "" | ||
| return f"{major}{minor}" |
There was a problem hiding this comment.
I can imagine that it would be useful to add a test for this implementation too.
| "split": split_filter, | ||
| } | ||
| ) | ||
| return env |
There was a problem hiding this comment.
All the more reason tho have an actual recipe that contains some of these corner cases.
|
This is a list of all jinja functionality supported in the recipe: conda/ceps#71 (@wolfv we should merge this) Do all of these work as expected now? |
|
Updated the missing filters and added env object. Also added mamba recipe to test that we really don't fail |
| from jinja2 import DebugUndefined | ||
|
|
||
|
|
||
| class _MissingUndefined(DebugUndefined): |
There was a problem hiding this comment.
Cant this error be returned by the functions to the user? In that case I think this thing shouldnt be private.
There was a problem hiding this comment.
I think you should in that case also either not put it in the utils module or reexport it.
There was a problem hiding this comment.
This error is not returned to the user ( jinja is raising directly jinja2.Undefined ) and it's something specific for our error handling - that's why I would prefer to keep private
tests/__snapshots__/test_jinja.ambr
Outdated
| outputs: | ||
| - build: | ||
| script: | ||
| - '' |
There was a problem hiding this comment.
This seems wrong it should be
- ${{ "build_mamba.sh" if unix }}
tests/__snapshots__/test_jinja.ambr
Outdated
| about: | ||
| description: '# Mamba, the Fast Cross-Platform Package Manager | ||
|
|
||
| ${{ env.get("MY_ENV_VAR")}} |
There was a problem hiding this comment.
Is the escaping correct here?
There was a problem hiding this comment.
it is quotation mark ( this one: " ) , I'm not exactly sure why syrupy keep it like this in snapshots
There was a problem hiding this comment.
fixed by asking jinja to not escape it :)
|
@baszalmstra @wolfv I'm setting similar shims as: https://github.com/wolfv/conda-smithy/blob/77cd535868932b088b3db7051cd1640655511343/conda_smithy/utils.py#L110 basically this means that we will render I couldn't find an easier way to preserve them ( chatgpt said that I need to write my own node parser, not sure if we need it right now ) Let me know if it's ok |
| if default: | ||
| return f"""${{{{ env.get("{env_var}", default="{default}") }}}}""" | ||
|
|
||
| return f"""${{{{ env.get("{env_var}")}}}}""" |
There was a problem hiding this comment.
If I understand correctly, this just returns the jinja string as is? Fine with me.
There was a problem hiding this comment.
made them all to return just the simple string
|
|
||
|
|
||
| def _stub_subpackage_pin(*args, **kwargs) -> str: # noqa: ARG001, ANN003, ANN002 | ||
| return f"subpackage_pin {args[0]}" |
There was a problem hiding this comment.
I am not sure I understand the difference. Here you return simple string, but in other functions you return ${{ .... }}.
There was a problem hiding this comment.
made them all to return just the simple string
| env.filters.update( | ||
| { | ||
| "version_to_buildstring": _version_to_build_string, | ||
| "split": _split, |
There was a problem hiding this comment.
I actually don't know if this is implemented in Python Jinja or not. Is it missing from Python jinja?
There was a problem hiding this comment.
It was missing from python jinja
|
@wolfv made the stubs behave the same |
We are missing the use-case when jinja function is used in template - it will raise an Exception.
This PR aims to fix it.