Allow 'None' as a default_value for _yaml.node_getnode-get-default

9 files changed, 37 insertions, 25 deletions

diff --git a/buildstream/_loader.py b/buildstream/_loader.py
index 6d384ae2c..44f38e3f1 100644
--- a/buildstream/_loader.py
+++ b/buildstream/_loader.py
@@ -155,9 +155,9 @@ def extract_depends_from_node(data):
         elif isinstance(dep, Mapping):
             _yaml.node_validate(dep, ['filename', 'type', 'junction'])
 
-            # Make type optional, for this we set it to None after
-            dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value="")
-            if not dep_type or dep_type == Symbol.ALL:
+            # Make type optional, for this we set it to None
+            dep_type = _yaml.node_get(dep, str, Symbol.TYPE, default_value=None)
+            if dep_type is None or dep_type == Symbol.ALL:
                 dep_type = None
             elif dep_type not in [Symbol.BUILD, Symbol.RUNTIME]:
                 provenance = _yaml.node_get_provenance(dep, key=Symbol.TYPE)
@@ -167,9 +167,7 @@ def extract_depends_from_node(data):
 
             filename = _yaml.node_get(dep, str, Symbol.FILENAME)
 
-            junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value="")
-            if not junction:
-                junction = None
+            junction = _yaml.node_get(dep, str, Symbol.JUNCTION, default_value=None)
 
             dependency = Dependency(filename,
                                     dep_type=dep_type, junction=junction,
@@ -584,11 +582,9 @@ class Loader():
             del source[Symbol.KIND]
 
             # Directory is optional
-            directory = _yaml.node_get(source, str, Symbol.DIRECTORY, default_value='')
+            directory = _yaml.node_get(source, str, Symbol.DIRECTORY, default_value=None)
             if directory:
                 del source[Symbol.DIRECTORY]
-            else:
-                directory = None
 
             index = sources.index(source)
             meta_source = MetaSource(element_name, index, kind, source, directory)
diff --git a/buildstream/_options/option.py b/buildstream/_options/option.py
index dfc8d4040..9501a2bde 100644
--- a/buildstream/_options/option.py
+++ b/buildstream/_options/option.py
@@ -62,10 +62,10 @@ class Option():
     #                 the option
     def load(self, node):
         self.description = _yaml.node_get(node, str, 'description')
-        self.variable = _yaml.node_get(node, str, 'variable', default_value='') or None
+        self.variable = _yaml.node_get(node, str, 'variable', default_value=None)
 
         # Assert valid symbol name for variable name
-        if self.variable:
+        if self.variable is not None:
             p = _yaml.node_get_provenance(node, 'variable')
             _yaml.assert_symbol_name(p, self.variable, 'variable name')
 
diff --git a/buildstream/_options/optionpool.py b/buildstream/_options/optionpool.py
index 42b1baa9a..0bb366020 100644
--- a/buildstream/_options/optionpool.py
+++ b/buildstream/_options/optionpool.py
@@ -243,8 +243,8 @@ class OptionPool():
     # Return true if a conditional was processed.
     #
     def _process_one_node(self, node):
-        conditions = _yaml.node_get(node, list, '(?)', default_value=[]) or None
-        assertion = _yaml.node_get(node, str, '(!)', default_value='') or None
+        conditions = _yaml.node_get(node, list, '(?)', default_value=None)
+        assertion = _yaml.node_get(node, str, '(!)', default_value=None)
 
         # Process assersions first, we want to abort on the first encountered
         # assertion in a given dictionary, and not lose an assertion due to
diff --git a/buildstream/_project.py b/buildstream/_project.py
index f49d7aa84..1f8a7a483 100644
--- a/buildstream/_project.py
+++ b/buildstream/_project.py
@@ -411,7 +411,7 @@ class Project():
 
                 # Parse the host mount
                 path = _yaml.node_get(host_file_desc, str, 'path')
-                host_path = _yaml.node_get(host_file_desc, str, 'host_path', default_value='') or None
+                host_path = _yaml.node_get(host_file_desc, str, 'host_path', default_value=None)
                 optional = _yaml.node_get(host_file_desc, bool, 'optional', default_value=False)
                 mount = HostMount(path, host_path, optional)
 
diff --git a/buildstream/_workspaces.py b/buildstream/_workspaces.py
index da430b540..8b7458771 100644
--- a/buildstream/_workspaces.py
+++ b/buildstream/_workspaces.py
@@ -58,13 +58,8 @@ class Workspace():
     @classmethod
     def from_yaml_node(cls, node, project):
         path = _yaml.node_get(node, str, 'path')
-        last_successful = _yaml.node_get(node, str, 'last_successful', default_value='')
-        running_files = _yaml.node_get(node, dict, 'running_files', default_value={})
-
-        if last_successful == '':
-            last_successful = None
-        if running_files == {}:
-            running_files = None
+        last_successful = _yaml.node_get(node, str, 'last_successful', default_value=None)
+        running_files = _yaml.node_get(node, dict, 'running_files', default_value=None)
 
         return cls(path, project, last_successful, running_files)
 
diff --git a/buildstream/_yaml.py b/buildstream/_yaml.py
index 85410caf2..7cf743521 100644
--- a/buildstream/_yaml.py
+++ b/buildstream/_yaml.py
@@ -305,6 +305,18 @@ def node_get_provenance(node, key=None, indices=None):
     return provenance
 
 
+# Helper to use utils.sentinel without unconditional utils import,
+# which causes issues for completion.
+#
+# Local private, but defined here because sphinx appears to break if
+# it's not defined before any functions calling it in default kwarg
+# values.
+#
+def _get_sentinel():
+    from .utils import _sentinel
+    return _sentinel
+
+
 # node_get()
 #
 # Fetches a value from a dictionary node and checks it for
@@ -326,10 +338,10 @@ def node_get_provenance(node, key=None, indices=None):
 # Note:
 #    Returned strings are stripped of leading and trailing whitespace
 #
-def node_get(node, expected_type, key, indices=None, default_value=None):
+def node_get(node, expected_type, key, indices=None, default_value=_get_sentinel()):
     value = node.get(key, default_value)
     provenance = node_get_provenance(node)
-    if value is None:
+    if value is _get_sentinel():
         raise LoadError(LoadErrorReason.INVALID_DATA,
                         "{}: Dictionary did not contain expected key '{}'".format(provenance, key))
 
@@ -341,6 +353,10 @@ def node_get(node, expected_type, key, indices=None, default_value=None):
             value = value[index]
             path += '[{:d}]'.format(index)
 
+    # We want to allow None as a valid value for any type
+    if value is None:
+        return None
+
     if not isinstance(value, expected_type):
         # Attempt basic conversions if possible, typically we want to
         # be able to specify numeric values and convert them to strings,
diff --git a/buildstream/element.py b/buildstream/element.py
index 3f4f409dc..f63812f4d 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -289,7 +289,7 @@ class Element(Plugin):
 
         return None
 
-    def node_subst_member(self, node, member_name, default=None):
+    def node_subst_member(self, node, member_name, default=utils._sentinel):
         """Fetch the value of a string node member, substituting any variables
         in the loaded value with the element contextual variables.
 
diff --git a/buildstream/plugin.py b/buildstream/plugin.py
index fc81a5940..e6a6b60fe 100644
--- a/buildstream/plugin.py
+++ b/buildstream/plugin.py
@@ -194,7 +194,7 @@ class Plugin():
         provenance = _yaml.node_get_provenance(node, key=member_name)
         return str(provenance)
 
-    def node_get_member(self, node, expected_type, member_name, default=None):
+    def node_get_member(self, node, expected_type, member_name, default=utils._sentinel):
         """Fetch the value of a node member, raising an error if the value is
         missing or incorrectly typed.
 
diff --git a/buildstream/utils.py b/buildstream/utils.py
index 209e9d094..40705346b 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -532,6 +532,11 @@ def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
         raise
 
 
+# A sentinel to be used as a default argument for functions that need
+# to distinguish between a kwarg set to None and an unset kwarg.
+_sentinel = object()
+
+
 # Recursively remove directories, ignoring file permissions as much as
 # possible.
 def _force_rmtree(rootpath, **kwargs):