New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] Merging of sibling nodes with same name is faulty #32
Comments
Names must be unique, so we can't allow multiple with the same name. It's used in a variety of places as a key, such as pathing in the tree, serialization, or simply getting values out of the context. Merging is supported as a design principle (which I really must write down somewhere!). It must be supported to "enhance" a command after it has already been registered, to allow mods to easily extend commands in Minecraft (and I'm sure other applications/games will find that useful too). That said, merging must be made smarter - I totally agree with this. We should probably do some sanity checking when a merge happens, and throw an exception if it's nonsense - if anything other than the children changed. For example in your code, the second |
What do you think about having separate methods for this modification instead? For example
Methods like these might be less suprising than runtime exceptions which are thrown when the merge fails and they already define restrictions during compile time. Additionally this implementation might be less error prone since there are a lot of cases you have to consider when determining if a merge is valid. |
A solution I can think of would be :
Exemple :// Case 1 :
dispatcher.register(
literal("foo")
.then(
literal("bar")
.executes(c -> {
System.out.println("string");
return 1;
})
)
.then(
literal("bar") // Same name, literal -> Merge
.then(literal("baz"))
)
);
// Case 2 :
dispatcher.register(
literal("foo")
.then(
argument("bar", word())
.executes(c -> {
System.out.println("string");
return 1;
})
)
.then(
argument("bar", word()) // Same name, same type -> Merge
.then(literal("baz"))
)
);
// Case 3 :
dispatcher.register(
literal("foo")
.then(
argument("bar", word())
.executes(c -> {
System.out.println("string");
return 1;
})
)
.then(
argument("bar", bool()) // Same name, different type -> Exception
.executes(c -> {
System.out.println("bool");
return 1;
})
)
); |
It appears it is intended that sibling nodes with the same name exist (see this comment). However the way they are treated is in my opinion rather unintuitive.
The method
CommandNode.addChild(CommandNode<S>)
replaces the command of the existing node and adds the children of the new node to the existing one. This is, at least from my point of view, not the wanted behavior sinceIt might be best to either remove this merging and allow siblings with the same name, or to prevent them when building.
Example code
The text was updated successfully, but these errors were encountered: