feat(schema): Enforce unique node names within a network - #751
Conversation
jetuk
left a comment
jetuk
left a comment
There was a problem hiding this comment.
The changes look good to me. I would just like to discuss whether we should add this to the core builder infrastructure in some fashion.
| /// not fail on its own: it silently binds every reference to whichever entry appears first | ||
| /// and leaves the other unreachable. | ||
| /// | ||
| /// `pywr-core` cannot catch this for us. Its duplicate check runs over the *expanded* core |
There was a problem hiding this comment.
The only question to ask ourselves is whether this should be caught in pywr-core during build. It would be possible to use a namespace builder (or something) to permit the construction of only one parent node name.
I guess is it more or less complex to do it there. Is the addition of a validation step going to confusing or difficult to maintain.
| /// `pywr-core` cannot catch this for us. Its duplicate check runs over the *expanded* core | ||
| /// nodes, keyed by name **and** sub-name. One schema node may expand to several core nodes, | ||
| /// and composite types such as [`crate::nodes::DelayNode`] emit only sub-named ones. | ||
| pub fn validate(&self) -> Result<(), SchemaError> { |
There was a problem hiding this comment.
I might argue if we do this that we should separate the error types between validation and add_to_network ValidationError.
| /// Every entry of [`NetworkSchema::nodes`] and [`NetworkSchema::virtual_nodes`] must have a | ||
| /// unique name. | ||
| /// | ||
| /// Duplicates must be rejected here because name resolution in this crate is |
There was a problem hiding this comment.
This reads a bit like internal documentation, not documentation of the function itself.
2 participants
Because node names in pywr-core are based on both the name and sub-name attrs and because schema nodes can add multiple nodes to core it is possible for two schema nodes with the same name to be added to core without raising an error. This can result in a misleading error such as
UnresolvedNode. This PR solves this by checking for duplicates in the schema, though there might be a better way of doing this.