juergbi commented on code in PR #2202:
URL: https://github.com/apache/buildstream/pull/2202#discussion_r4425174239
##########
src/buildstream/node.pyx:
##########
@@ -977,6 +977,9 @@ cdef class MappingNode(Node):
if type(target_value) is SequenceNode:
# Composite list composes into a list
self._compose_on_list(target_value)
+ elif isinstance((<ScalarNode> target_value).value, str):
Review Comment:
Doesn't this require a type check before the cast to `ScalarNode`?
##########
src/buildstream/node.pyx:
##########
@@ -1016,6 +1019,26 @@ cdef class MappingNode(Node):
if suffix is not None:
target.value.extend(suffix.value)
+ # _compose_on_string(target)
+ #
+ # Compose the current node on the given string.
+ #
+ # Args:
+ # target (.SequenceNode): sequence on which to compose the current
composite dict
+ #
+ cdef void _compose_on_string(self, ScalarNode target):
+ cdef ScalarNode clobber = self.value.get("(=)")
+ cdef ScalarNode prefix = self.value.get("(<)")
+ cdef ScalarNode suffix = self.value.get("(>)")
+
+ if clobber is not None:
+ target.value.clear()
+ target.value.extend(clobber.value)
Review Comment:
This doesn't look right for strings. I'd expect a simple assignment to work
fine, though.
```suggestion
target.value = clobber.value
```
##########
src/buildstream/node.pyx:
##########
@@ -1016,6 +1019,26 @@ cdef class MappingNode(Node):
if suffix is not None:
target.value.extend(suffix.value)
+ # _compose_on_string(target)
+ #
+ # Compose the current node on the given string.
+ #
+ # Args:
+ # target (.SequenceNode): sequence on which to compose the current
composite dict
Review Comment:
Nit: Not a `SequenceNode`
##########
src/buildstream/node.pyx:
##########
@@ -977,6 +977,9 @@ cdef class MappingNode(Node):
if type(target_value) is SequenceNode:
# Composite list composes into a list
self._compose_on_list(target_value)
+ elif isinstance((<ScalarNode> target_value).value, str):
+ # Composite list composes into a string
+ self._compose_on_string(target_value)
elif target_value._is_composite_list():
# Composite list merges into composite list
self._compose_on_composite_dict(target_value)
Review Comment:
`_compose_on_composite_dict` only knows about list, not string composition.
Couldn't this be hit via multiple includes? It may be acceptable to start with
this as known limitation, if there is a reasonable error.
##########
doc/source/format_intro.rst:
##########
@@ -264,10 +269,24 @@ instead of the default behavior which is to replace the
target list.
.. _format_directives_list_append:
-(>) List Append
+For strings:
Review Comment:
This is an example for prepend, so it should be above the
`_format_directives_list_append` marker (and I think the
`_format_directives_string_append` marker is unused.
##########
src/buildstream/node.pyx:
##########
@@ -1016,6 +1019,26 @@ cdef class MappingNode(Node):
if suffix is not None:
target.value.extend(suffix.value)
+ # _compose_on_string(target)
+ #
+ # Compose the current node on the given string.
+ #
+ # Args:
+ # target (.SequenceNode): sequence on which to compose the current
composite dict
+ #
+ cdef void _compose_on_string(self, ScalarNode target):
+ cdef ScalarNode clobber = self.value.get("(=)")
+ cdef ScalarNode prefix = self.value.get("(<)")
+ cdef ScalarNode suffix = self.value.get("(>)")
+
+ if clobber is not None:
+ target.value.clear()
+ target.value.extend(clobber.value)
+ if prefix is not None:
+ target.value = prefix.value + target.value
+ if suffix is not None:
+ target.value += suffix.value
Review Comment:
As `ScalarNode` strips whitespace from the start and end of strings (even if
they're quoted), it won't be possible to append e.g. " -foo" to a flags
variable, as far as I can tell. Not sure what the best solution is for this,
especially considering backwards compatibility, but this might block real use
cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
