Skip to content
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

Improve messaging for templates processor #1521

Merged

Conversation

sfc-gh-melnacouzi
Copy link
Contributor

@sfc-gh-melnacouzi sfc-gh-melnacouzi commented Aug 30, 2024

Pre-review checklist

  • I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
  • I've added or updated automated unit tests to verify correctness of my new code.
  • I've added or updated integration tests to verify correctness of my new code.
  • I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
  • I've confirmed that my changes are working by executing CLI's commands manually on Windows.
  • I've confirmed that my changes are up-to-date with the target branch.
  • I've described my changes in the release notes.
  • I've described my changes in the section below.

Changes description

Improve messaging for templates processor:

  1. Improve artifact processor printing.
  2. Add a message every time a file is templated and it's expanded:
Invoking artifact processors
  Processing artifact group [app/* -> ./] with templates processor
    Expanding templates in app/setup_script.sql

Update:
Now it's a bit different, after discussion:

Invoking artifact processors
  Processing Templates from app/dir/test
  Processing Templates from app/setup_script.sql

@sfc-gh-melnacouzi sfc-gh-melnacouzi requested a review from a team as a code owner August 30, 2024 19:24
Comment on lines 88 to 90
cc.step(
f"Processing artifact group [{artifact_to_process.src} -> {artifact_to_process.dest}] with templates processor"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason why this should be different from the snowpark processor, which simply uses something like this:

Invoking artifact processors
    Processing Snowpark annotations from python/receipts.py
    Processing Snowpark annotations from python/regions.py

I think that from the POV of the user, too much information ends up being noise. What we're really trying to convey is the transformation that's happening for each file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it to be consistent.

if src.is_dir():
return
with self.edit_file(dest) as file:
file_name = src.relative_to(self._bundle_ctx.project_root)
Copy link
Contributor

@sfc-gh-bdufour sfc-gh-bdufour Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor, but when we chain processors, there might not be a source in the case of generated files.

In any case, the Snowpark processor outputs the dest path (relative to the deploy root), not the src path. At the very they should both align, so consider changing the Snowpark processor logic to match yours.

raise InvalidTemplateInFileError(file_name, e) from e

if expanded_template != file.contents:
cc.message(f"Expanding templates in {file_name}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too late, since there could be errors raised before this is output. I get that you don't want to say anything about non-templated files, but we should then at least buffer the exceptions until we have a chance to tell the user what step is about to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but since all errors will have the file name included, it might not be worth it. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely worth it IMHO. We should always tell the user what we're doing, and do so accurately.

@sfc-gh-melnacouzi sfc-gh-melnacouzi requested a review from a team as a code owner September 3, 2024 14:14

def test_has_sql_templates():
assert has_sql_templates("abc <% %> abc")
assert has_sql_templates("abc <% abc")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example does not have SQL template, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Jinja perspective it's considered it has it, and it will fail when expanding.

…es_processor.py

Co-authored-by: Bruno Dufour <bruno.dufour@snowflake.com>
@sfc-gh-melnacouzi sfc-gh-melnacouzi enabled auto-merge (squash) September 9, 2024 13:22
@sfc-gh-melnacouzi sfc-gh-melnacouzi merged commit d3de8bb into main Sep 9, 2024
18 checks passed
@sfc-gh-melnacouzi sfc-gh-melnacouzi deleted the melnacouzi-fix-messaging-for-templates-processor branch September 9, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants