databricks / cli

Databricks CLI
Other
115 stars 39 forks source link

Return `dyn.InvalidValue` instead of `dyn.NilValue` when errors happen #1514

Closed shreyas-goenka closed 1 week ago

shreyas-goenka commented 1 week ago

Changes

With https://github.com/databricks/cli/pull/1507 and https://github.com/databricks/cli/pull/1511 we are clarifying the semantics associated with dyn.InvalidValue and dyn.NilValue. An invalid value is the default zero value and is used to signals the complete absence of the value.

A nil value, on the other hand, is a valid value for a piece of configuration and signals explicitly setting a key to nil in the configuration tree. In keeping with that theme, this PR returns dyn.InvalidValue instead of dyn.NilValue at error sites. This change is not expected to have a material change in behaviour and is being done to set the right convention since we have well-defined semantics associated with both NilValue and InvalidValue.

Tests

Unit tests and integration tests pass. Also manually scanned the changes and the associated call sites to verify the NilValue value itself was not being relied upon.

shreyas-goenka commented 1 week ago

gopatch file used:

@@
@@
-return dyn.NilValue, err
+return dyn.InvalidValue, err

@@
@@
-return dyn.NilValue, fmt.Errorf(...)
+return dyn.InvalidValue, fmt.Errorf(...)

@@
@@
-return dyn.NilValue, errorf(...)
+return dyn.InvalidValue, errorf(...)