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

Remove project section check outside with_project_definition decorator #1276

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

sfc-gh-melnacouzi
Copy link
Contributor

@sfc-gh-melnacouzi sfc-gh-melnacouzi commented Jul 2, 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

Refactor with_project_definition so that section checks are done outside of the decorator code.

  • visible changes:
    1. when snowflake.yml is not found, the message is generic (e.g. Snowflake project instead of the Native App project)
    1. when section is not found, use project_root in the message (absolute path of snowflake.yml folder) instead of the value of the optional -p param.

Release notes not updated because there are no significant changes from the point of view of users.

This refactor was requested by @sfc-gh-turbaszek earlier. We might be introducing more code duplication with this refactor, so I would like to confirm with @sfc-gh-turbaszek if this is still needed.

@sfc-gh-melnacouzi sfc-gh-melnacouzi requested review from a team as code owners July 2, 2024 18:26
Comment on lines 157 to 160
if cli_context.project_definition.native_app is None:
raise NoProjectDefinitionError(
project_type="native_app", project_file=cli_context.project_root
)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about wrapping this in an assert_native_app_pdf or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added util function

@sfc-gh-bdufour
Copy link
Contributor

What's the motivation behind this change? There's definitely a tradeoff happening here.

Comment on lines 154 to 157
if cli_context.project_definition.native_app is None:
raise NoProjectDefinitionError(
project_type="native_app", project_file=cli_context.project_root
)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about introducing a helper
assert_project_type(project_type="native_app")

this would reduce the code duplication. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sfc-gh-turbaszek
Copy link
Contributor

What's the motivation behind this change? There's definitely a tradeoff happening here.

Introduction of env support made all commands using project file to require full project definition. To do so we already changed with_project_definition. Now, the proposed change make sure that the decorator provides the definition (full, not a single part like app or streamlit). It's also opens the code for further changes related to snowflake.yml V2.

@sfc-gh-melnacouzi sfc-gh-melnacouzi force-pushed the melnacouzi-refactor-project-name branch 2 times, most recently from 1c3b147 to 2c36e62 Compare July 3, 2024 16:45
@sfc-gh-melnacouzi sfc-gh-melnacouzi enabled auto-merge (squash) July 3, 2024 17:32
@sfc-gh-turbaszek sfc-gh-turbaszek enabled auto-merge (squash) July 11, 2024 12:17
@sfc-gh-turbaszek sfc-gh-turbaszek merged commit 8938645 into main Jul 11, 2024
16 checks passed
@sfc-gh-turbaszek sfc-gh-turbaszek deleted the melnacouzi-refactor-project-name branch July 11, 2024 12:32
sfc-gh-sichen pushed a commit that referenced this pull request Oct 17, 2024
#1276)

* Remove project section check outside with_project_definition decorator

* Add util function for checking project type
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.

4 participants