Clean up TODOs

Snowflake-Labs · Dec 8, 2024 · b5123b1 · b5123b1
1 parent 474e42d
commit b5123b1
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 44 deletions.
diff --git a/pkg/acceptance/helpers/function_client.go b/pkg/acceptance/helpers/function_client.go
@@ -44,7 +44,7 @@ func (c *FunctionClient) CreateWithIdentifier(t *testing.T, id sdk.SchemaObjectI
 	)
 }
 
-// TODO [next PR]: improve this helper (all  other types creation)
+// TODO [SNOW-1850370]: improve this helper (all  other types creation)
 func (c *FunctionClient) CreateSecure(t *testing.T, arguments ...sdk.DataType) *sdk.Function {
 	t.Helper()
 	id := c.ids.RandomSchemaObjectIdentifierWithArguments(arguments...)

diff --git a/pkg/sdk/functions_ext.go b/pkg/sdk/functions_ext.go
@@ -147,26 +147,3 @@ func NewCreateForJavascriptFunctionRequestDefinitionWrapped(
 	s.FunctionDefinition = fmt.Sprintf(`$$%s$$`, FunctionDefinition)
 	return &s
 }
-
-// CreateForJavaFunctionOptions
-// TODO [SNOW-1348103 - this PR]: test setting the paths for all types (like imports, target paths)
-// TODO [SNOW-1348103 - this PR]: test weird names for arg name - lower/upper if used with double quotes, to upper without quotes, dots, spaces, and both quotes not permitted
-// TODO [SNOW-1348103 - next PRs]: check data type mappings https://docs.snowflake.com/en/sql-reference/sql/create-function#all-languages (signature + returns)
-// TODO [SNOW-1348103 - this PR]: setting RUNTIME_VERSION (only 11.x, 17.x supported, 11.x being the default)
-// TODO [SNOW-1348103 - this PR]: packages: package_name:version_number; do we validate? - check SELECT * FROM INFORMATION_SCHEMA.PACKAGES WHERE LANGUAGE = 'java';
-// TODO [SNOW-1348103 - next PRs]: add to the resource docs https://docs.snowflake.com/en/sql-reference/sql/create-function#access-control-requirements
-// TODO [SNOW-1348103 - this PR]: what delimiter do we use for <function_definition>: ' versus $$? - we use $$ as tasks
-// TODO [SNOW-1348103 - this PR]: escaping single quotes test - don't have to do this with $$
-// TODO [SNOW-1348103 - this PR]: validation of JAR (check https://docs.snowflake.com/en/sql-reference/sql/create-function#id6)
-// TODO [SNOW-1348103 - next PRs]: active warehouse vs validations
-// TODO [SNOW-1348103 - this PR]: check creation of all functions (using examples and more)
-
-// CreateForPythonFunctionOptions
-// TODO [SNOW-1348103 - this PR]: test aggregate func creation
-// TODO [SNOW-1348103 - this PR]: what about [==<version>] - SDK level or resource level? check also: SELECT * FROM INFORMATION_SCHEMA.PACKAGES WHERE LANGUAGE = 'python';
-// TODO [SNOW-1348103 - this PR]: what about preview feature >= ?
-// TODO [SNOW-1348103 - this PR]: what about '<module_file_name>.<function_name>' for non-inline functions?
-// TODO [SNOW-1348103 - this PR]: setting RUNTIME_VERSION (only 3.8, 3.9, 3.10, 3.11 supported, which one is a default?)
-
-// CreateForScalaFunctionOptions
-// TODO [SNOW-1348103 - this PR]: setting RUNTIME_VERSION (only 2.12 supported, which is the default)
diff --git a/pkg/sdk/testint/functions_integration_test.go b/pkg/sdk/testint/functions_integration_test.go
@@ -20,17 +20,19 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TODO [next PR]: schemaName and catalog name are quoted (because we use lowercase)
-// TODO [next PR]: HasArgumentsRawFrom(functionId, arguments, return)
-// TODO [next PR]: extract show assertions with commons fields
-// TODO [next PR]: test confirming that runtime version is required for Scala function
-// TODO [next PR]: test create or replace with name change, args change
-// TODO [next PR]: test rename more (arg stays, can't change arg, rename to different schema)
-// TODO [next PR]: add test documenting that UNSET SECRETS does not work
-// TODO [next PR]: add test documenting [JAVA]: 391516 (42601): SQL compilation error: Cannot specify TARGET_PATH without a function BODY.
-// TODO [next PR]: test secure
-// TODO [next PR]: python aggregate func (100357 (P0000): Could not find accumulate method in function CVVEMHIT_06547800_08D6_DBCA_1AC7_5E422AFF8B39 with handler dump)
-// TODO [next PR]: add a test documenting that we can't set parameters in create (and revert adding these parameters directly in object...)
+// TODO [SNOW-1348103]: schemaName and catalog name are quoted (because we use lowercase)
+// TODO [SNOW-1850370]: HasArgumentsRawFrom(functionId, arguments, return)
+// TODO [SNOW-1850370]: extract show assertions with commons fields
+// TODO [SNOW-1850370]: test confirming that runtime version is required for Scala function
+// TODO [SNOW-1348103 or SNOW-1850370]: test create or replace with name change, args change
+// TODO [SNOW-1348103]: test rename more (arg stays, can't change arg, rename to different schema)
+// TODO [SNOW-1348103]: test weird names for arg name - lower/upper if used with double quotes, to upper without quotes, dots, spaces, and both quotes not permitted
+// TODO [SNOW-1850370]: add test documenting that UNSET SECRETS does not work
+// TODO [SNOW-1850370]: add test documenting [JAVA]: 391516 (42601): SQL compilation error: Cannot specify TARGET_PATH without a function BODY.
+// TODO [SNOW-1348103 or SNOW-1850370]: test secure
+// TODO [SNOW-1348103]: python aggregate func (100357 (P0000): Could not find accumulate method in function CVVEMHIT_06547800_08D6_DBCA_1AC7_5E422AFF8B39 with handler dump)
+// TODO [SNOW-1348103]: add a test documenting that we can't set parameters in create (and revert adding these parameters directly in object...)
+// TODO [SNOW-1850370]: active warehouse vs validations
 func TestInt_Functions(t *testing.T) {
 	client := testClient(t)
 	ctx := context.Background()
@@ -207,8 +209,8 @@ func TestInt_Functions(t *testing.T) {
 			HasNullHandling(string(sdk.NullInputBehaviorReturnNullInput)).
 			HasVolatility(string(sdk.ReturnResultsBehaviorImmutable)).
 			HasExternalAccessIntegrations(fmt.Sprintf(`[%s]`, externalAccessIntegration.FullyQualifiedName())).
-			// TODO [next PR]: parse to identifier list
-			// TODO [next PR]: check multiple secrets (to know how to parse)
+			// TODO [SNOW-1348103]: parse to identifier list
+			// TODO [SNOW-1348103]: check multiple secrets (to know how to parse)
 			HasSecrets(fmt.Sprintf(`{"abc":"\"%s\".\"%s\".%s"}`, secretId.DatabaseName(), secretId.SchemaName(), secretId.Name())).
 			HasImports(fmt.Sprintf(`[%s]`, tmpJavaFunction.JarLocation())).
 			HasHandler(handler).
@@ -565,7 +567,7 @@ func TestInt_Functions(t *testing.T) {
 
 		assertions.AssertThatObject(t, objectassert.FunctionDetails(t, function.ID()).
 			HasSignature(fmt.Sprintf(`(%s %s)`, argName, dataType.ToLegacyDataTypeSql())).
-			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")). //TODO [next PR]: do we care about this whitespace?
+			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")). //TODO [SNOW-1348103]: do we care about this whitespace?
 			HasLanguage("PYTHON").
 			HasBody(definition).
 			HasNullHandling(string(sdk.NullInputBehaviorCalledOnNullInput)).
@@ -647,7 +649,7 @@ func TestInt_Functions(t *testing.T) {
 
 		assertions.AssertThatObject(t, objectassert.FunctionDetails(t, function.ID()).
 			HasSignature(fmt.Sprintf(`(%s %s)`, argName, dataType.ToLegacyDataTypeSql())).
-			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")+" NOT NULL"). //TODO [next PR]: do we care about this whitespace?
+			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")+" NOT NULL"). // TODO [SNOW-1348103]: do we care about this whitespace?
 			HasLanguage("PYTHON").
 			HasBody(definition).
 			HasNullHandling(string(sdk.NullInputBehaviorReturnNullInput)).
@@ -714,7 +716,7 @@ func TestInt_Functions(t *testing.T) {
 
 		assertions.AssertThatObject(t, objectassert.FunctionDetails(t, function.ID()).
 			HasSignature(fmt.Sprintf(`(%s %s)`, argName, dataType.ToLegacyDataTypeSql())).
-			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")). //TODO [next PR]: do we care about this whitespace?
+			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")).
 			HasLanguage("PYTHON").
 			HasBodyNil().
 			HasNullHandling(string(sdk.NullInputBehaviorCalledOnNullInput)).
@@ -793,7 +795,7 @@ func TestInt_Functions(t *testing.T) {
 
 		assertions.AssertThatObject(t, objectassert.FunctionDetails(t, function.ID()).
 			HasSignature(fmt.Sprintf(`(%s %s)`, argName, dataType.ToLegacyDataTypeSql())).
-			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")+" NOT NULL"). //TODO [next PR]: do we care about this whitespace?
+			HasReturns(strings.ReplaceAll(dataType.ToSql(), " ", "")+" NOT NULL").
 			HasLanguage("PYTHON").
 			HasBodyNil().
 			HasNullHandling(string(sdk.NullInputBehaviorReturnNullInput)).
@@ -1245,7 +1247,7 @@ func TestInt_Functions(t *testing.T) {
 			HasLanguage("SQL").
 			HasBody(definition).
 			HasNullHandlingNil().
-			// TODO [next PR]: volatility is not returned and is present in create syntax
+			// TODO [SNOW-1348103]: volatility is not returned and is present in create syntax
 			//HasVolatility(string(sdk.ReturnResultsBehaviorImmutable)).
 			HasVolatilityNil().
 			HasExternalAccessIntegrationsNil().
@@ -1445,7 +1447,7 @@ func TestInt_Functions(t *testing.T) {
 
 		assertions.AssertThatObject(t, objectassert.FunctionDetails(t, id).
 			HasExternalAccessIntegrationsNil().
-			// TODO [next PR]: apparently UNSET external access integrations cleans out secrets in the describe but leaves it in SHOW
+			// TODO [SNOW-1850370]: apparently UNSET external access integrations cleans out secrets in the describe but leaves it in SHOW
 			HasSecretsNil(),
 		)
 

diff --git a/pkg/sdk/testint/setup_test.go b/pkg/sdk/testint/setup_test.go
@@ -162,7 +162,7 @@ func (itc *integrationTestContext) initialize() error {
 
 	itc.testClient = helpers.NewTestClient(c, TestDatabaseName, TestSchemaName, TestWarehouseName, random.IntegrationTestsSuffix)
 
-	// TODO [next PR]: improve setup; this is a quick workaround for faster local testing
+	// TODO [SNOW-1763603]: improve setup; this is a quick workaround for faster local testing
 	if os.Getenv(string(testenvs.SimplifiedIntegrationTestsSetup)) == "" {
 		config, err := sdk.ProfileConfig(testprofiles.Secondary)
 		if err != nil {