space-wizards
Make ParsedValueRef public
Currently, in a CommandArgumentBundle the stored Arguments are of type ParsedValueRef when you check them inside a CustomTypeParser. This makes it very annoying to get the types parsed by the previous parsers inside a CustomTypeParser (you would need to manually call Evaluate with a Invocation Context, very annoying).
With this change, the resulting code would instead cast the objects inside of Arguments to ParsedValueRef where you can then check the Value contained for what type you need.
As for concerns regarding making it internal, the class has 2 usages, none of which seem security critical if exposed.
This makes it very annoying to get the types parsed by the previous parsers inside a
CustomTypeParser(you would need to manually call Evaluate with a Invocation Context, very annoying).
This sounds extremely questionable to do in the first place. What is your use case?
For https://github.com/space-wizards/space-station-14/pull/41455 I helped Fildrance make them toolshed. One of the arguments requires filtering based on a previously given argument.
[CommandImplementation("create_node_at_depth")]
public void CreateNodeAtDepth(
[CommandArgument] Entity<XenoArtifactComponent> artifact,
[CommandArgument(typeof(XenoEffectParser))] ProtoId<EntityPrototype> effect,
[CommandArgument] ProtoId<XenoArchTriggerPrototype> trigger,
[CommandArgument(typeof(XenoArtifactNodeParser))] Entity<XenoArtifactNodeComponent> node)
{
CreateNode(artifact, effect, trigger, node);
}
The parser XenoArtifactNodeParser filters the available entities based on what nodes are in the previously given Entity<XenoArtifactComponent>
For this I have come up with the following solution:
private Entity<XenoArtifactComponent>? GetPreviousArtifact(ParserContext ctx)
{
// We first try to find the argument in our previous chain that takes in a Entity<XenoArtifactComponent>
// If we can't find one this parser is being used incorrectly.
if (ctx.Bundle.Arguments == null)
{
return null;
}
Entity<XenoArtifactComponent>? entity = null;
foreach (var (_, value) in ctx.Bundle.Arguments)
{
if (value is not ParsedValueRef<Entity<XenoArtifactComponent>> valueRef)
continue;
entity = valueRef.Value;
break;
}
DebugTools.AssertNotNull(entity);
return entity;
}
I am not sure if there is a better way. Especially because Arguments is a Dictionary of type <string, object>. Without this PR I could only cast it ValueRef which means I can only access Evaluate and that would then require a dummy invocation context. Not very pretty. You can find both patches in the Discord quick review thread
One of the arguments requires filtering based on a previously given argument.
This filtering should surely be performed by the command, not the parser, no?
This is for the sake of autocomplete. Right now I could replace it with a Entity<T> command argument without a custom parser. This would then however show all entities of T, however in this specific usecase, you cannot put in just any entity.
This is for the sake of autocomplete.
In the PR, your TryParse implementation totally uses this too, though?
Anyways, I asked Kaylie and she said there's various issues in practice due to this not being a good way to do it. Probably best to discuss with her what the way forward is. This does look quite hacky.
In the PR, your
TryParseimplementation totally uses this too, though?
Well yes, because it makes sense. TryParse uses the same logic to verify, TryAutocomplete uses it to show the available artifact nodes based on the artifact given. It is quite hacky, but I am not aware of any other way to do it currently without expanding the scope to a point outside my skillset.
Well yes, because it makes sense. TryParse uses the same logic to verify
No, it does not. What if your parser doesn't run at all, because a variable is used instead?
I haven't actually thought about that, good catch. But in any case, I would still want advise on how else you would achieve this behavior. For autocomplete that is.
But in any case, I would still want advise on how else you would achieve this behavior. For autocomplete that is.
Anyways, I asked Kaylie and she said there's various issues in practice due to this not being a good way to do it. Probably best to discuss with her what the way forward is. This does look quite hacky.