Snow 1625040 unify sql template syntax #1458
Conversation
| template = env.get_template(relpath) | ||
| result = template.render(**jinja_context) | ||
| template_content = script_full_path.read_text( | ||
| file_size_limit_mb=DEFAULT_SIZE_LIMIT_MB |
There was a problem hiding this comment.
@sfc-gh-melnacouzi if we want to omit this check, we can use SecurePath.UNLIMITED here
There was a problem hiding this comment.
For SQL files, 128MB is a lot. It's probably ok to keep it here as long as the error message to the user is clear. I am fine with UNLIMITED as well, which is safer for backward compatibility
| f" and {_SQL_TEMPLATE_START} ... {_SQL_TEMPLATE_END} syntax." | ||
| ) | ||
| if has_old_syntax: | ||
| return old_syntax_env |
There was a problem hiding this comment.
do we want a warning here?
| Returns: | ||
| - List of expanded scripts content. | ||
| - List of rendered scripts content | ||
| Size of the return list is the same as the size of the input scripts list. | ||
| """ | ||
| scripts_contents = [] | ||
| for relpath in scripts: |
There was a problem hiding this comment.
nit: it's not always relative path, so maybe we can call it script_path
There was a problem hiding this comment.
I'm confused about this one: previously the function worked on jinja.Environment, which in both usages had FileLoader set on project_root (also in L599 relpath suggests that input paths should be relative). In current implementation the function won't work for global paths - should we change it? Do we currently have a use case for global paths?
| template = env.get_template(relpath) | ||
| result = template.render(**jinja_context) | ||
| template_content = script_full_path.read_text( | ||
| file_size_limit_mb=DEFAULT_SIZE_LIMIT_MB |
There was a problem hiding this comment.
For SQL files, 128MB is a lot. It's probably ok to keep it here as long as the error message to the user is clear. I am fine with UNLIMITED as well, which is safer for backward compatibility
|
|
||
| def path_to_jinja_pathlike_str(path: Path) -> str: | ||
| # jinja2 template loader user '/' as path separator (even on Windows) | ||
| return "/".join(path.parts) |
There was a problem hiding this comment.
have you tried path.to_posix() instead? It seems to work well for me.
* Add <% ... %> syntax to SQL rendering * add unit tests * fix nativeapp rendering * update release notes * Add integration tests * update nativeapp unit tests * Fix Windows paths * self-review * Change SQL rendering to choose syntax depending on template * revert nativeapp changes * refactor nativeapp usage * use SecurePath to open files
* Add <% ... %> syntax to SQL rendering * add unit tests * fix nativeapp rendering * update release notes * Add integration tests * update nativeapp unit tests * Fix Windows paths * self-review * Change SQL rendering to choose syntax depending on template * revert nativeapp changes * refactor nativeapp usage * use SecurePath to open files
* Add <% ... %> syntax to SQL rendering * add unit tests * fix nativeapp rendering * update release notes * Add integration tests * update nativeapp unit tests * Fix Windows paths * self-review * Change SQL rendering to choose syntax depending on template * revert nativeapp changes * refactor nativeapp usage * use SecurePath to open files
Pre-review checklist
Changes description
Add support for
<% ... %>template syntax in SQL queries