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

SNOW-1331032 add retrieve func defaults from source #1957

Merged

Conversation

sfc-gh-aalam
Copy link
Contributor

@sfc-gh-aalam sfc-gh-aalam commented Jul 22, 2024

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1331032

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
  3. Please describe how your code solves the related issue.

    Splits original PR SNOW-1331032: Support default argument values in UDXF and SP #1509.

This PR adds a new function retrieve_func_defaults_from_source which extracts default values from function registration when registrining sproc, udf, udtf from a file. The function is similar to retrieve_func_defaults_from_source and parses the AST of the given file to extract the default values.

@sfc-gh-aalam sfc-gh-aalam added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Jul 22, 2024
@sfc-gh-aalam sfc-gh-aalam marked this pull request as ready for review July 22, 2024 22:51
@sfc-gh-aalam sfc-gh-aalam requested a review from a team as a code owner July 22, 2024 22:51
@sfc-gh-aalam sfc-gh-aalam requested review from sfc-gh-yixie, sfc-gh-aling and sfc-gh-yuwang and removed request for a team July 22, 2024 22:51
Copy link
Collaborator

@sfc-gh-yuwang sfc-gh-yuwang left a comment

Choose a reason for hiding this comment

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

Seems like a few lines are not covered by the test, can you add them?

@sfc-gh-aalam
Copy link
Contributor Author

Seems like a few lines are not covered by the test, can you add them?

I added more tests. Just a note that the full PR has coverage satisfied: #1509

Base automatically changed from aalam-SNOW-1331032-add-python-value-str-to-object to main July 26, 2024 22:57
Comment on lines +664 to +671
class {class_name}:
def {func_name}(self, x, y {datatype_str} = {annotated_value}) -> None:
return None
"""
else:
source = f"""
def {func_name}(x, y {datatype_str} = {annotated_value}) -> None:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: have you tested multiple default values? def func(a, b=xx, c=xxx, g=xx)
it doesn't need to be complicated, but just testing multiple kwargs also works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@github-actions github-actions bot added the local testing Local Testing issues/PRs label Aug 1, 2024
Copy link

github-actions bot commented Aug 1, 2024

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

@sfc-gh-aalam
Copy link
Contributor Author

@sfc-gh-aalam sfc-gh-aalam enabled auto-merge (squash) August 1, 2024 21:05
Copy link

github-actions bot commented Aug 1, 2024

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

@sfc-gh-aalam sfc-gh-aalam merged commit c47411a into main Aug 1, 2024
34 checks passed
@sfc-gh-aalam sfc-gh-aalam deleted the aalam-SNOW-1331032-add-retrieve_func_defaults_from_source branch August 1, 2024 21:37
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
local testing Local Testing issues/PRs NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants