Convert node to list so that they are processed in order. #507
Conversation
| @@ -59,7 +56,7 @@ def with_launch_prefixes( | |||
| nodes: Dict[str, NodeConfig] = {n.name: n for n in self.nodes} | |||
| for node_name, prefix in prefixes.items(): | |||
| nodes[node_name] = nodes[node_name].with_launch_prefix(prefix) | |||
| return attr.evolve(self, nodes=frozenset(nodes.values())) | |||
| return attr.evolve(self, nodes=list(nodes.values())) | |||
There was a problem hiding this comment.
prefer (immutable) tuple over list
| @@ -140,7 +137,7 @@ def with_node(self, node: NodeConfig) -> "LaunchConfig": | |||
| m = "multiple definitions of node [{}] in launch configuration" | |||
| m = m.format(node.full_name) | |||
| raise FailedToParseLaunchFile(m) | |||
| nodes = self.nodes | frozenset({node}) | |||
| nodes = self.nodes | list(node) | |||
There was a problem hiding this comment.
I haven't looked at the mypy output yet, but I suspect that this is the line that's causing it to fail. The set union operator doesn't make sense for a list here. This line can just be replaced by:
nodes = self.nodes + [node]There was a problem hiding this comment.
I completely misread what this line was doing. Changed it to extend the tuple
| nodes.add(node) | ||
| return attr.evolve(self, nodes=frozenset(nodes)) | ||
| nodes.append(node) | ||
| return attr.evolve(self, nodes=nodes) |
There was a problem hiding this comment.
beware: nodes is a list here; we probably want to convert it to a tuple before passing it to evolve (otherwise we'll get a crash on line 140)
| @@ -31,7 +31,7 @@ | |||
|
|
|||
| @attr.s(frozen=True, slots=True) | |||
| class LaunchConfig: | |||
| nodes: Sequence[NodeConfig] = attr.ib(default=()) | |||
| nodes: Tuple[NodeConfig, ...] = attr.ib(default=()) | |||
There was a problem hiding this comment.
why this change? (tuples are sequences)
There was a problem hiding this comment.
If I make it a Sequence, mypy doesn't like the '+' operation.
src/roswire/common/launch/config/launch.py:140: error: Unsupported left operand type for + ("Sequence[NodeConfig]")
Is there a way around this?
Nodes in a LaunchConfig were sets, which breaks the ordering required for starting nodelets after nodelet managers.