Condition Parameter Generation #567
Conversation
Specifically it is used for - UseAliases (as AliasIndex) - UsePackageData (as PackageDataIndex) - Event Data (as EventDataIndex)
Noggog
left a comment
Noggog
left a comment
There was a problem hiding this comment.
On a good track. Few more things to add, i think!
| #region Mutagen | ||
| public object? Parameter1 | ||
| { | ||
| get => null; |
There was a problem hiding this comment.
Getting null here feels fine. Feels a bit odd to just do nothing in the set. Gut is telling me we should throw if the given value is not null (similar to how we throw if the type is wrong for used parameters)
There was a problem hiding this comment.
I was worrying this could lead to strange behavior when you are working on generic conditions. Setting a property might not be intuitively something where you expect it throws when doing it? What do you think? You can of course just add a try catch block when you get any issues with this.
There was a problem hiding this comment.
Yeah, not a great way either route... but I think that's just the price of writing generic API that is trying to handle everything.
The downside of the current setup is that users will expect that if they set the field and it "went through", that something was modified. But instead, it didnt modify anything, and was just a "silent error" of sorts.
The throw at least is something the user can detect/handle, vs the silent error just leads to more of an undetectable paranoia, imo
| public object? Parameter1 | ||
| { | ||
| get => Faction; | ||
| set => Faction = (value is IFormLinkOrIndex<IFactionGetter> v ? v : throw new ArgumentException()); |
There was a problem hiding this comment.
I like the throw if the type is wrong.
Might be nice to add Type Parameter1Type { get; } parameter so that the user can know/expect what type should be given?
| sb.AppendLine("set"); | ||
| using (sb.CurlyBrace()) | ||
| { | ||
| sb.AppendLine(); |
There was a problem hiding this comment.
Putting it here too, but we shuld check if it's not null and throw argument exception if it's not null?
There was a problem hiding this comment.
I think this is a similar design decision as throwing the other exception. Should be both or none.
| if (i > 2) break; | ||
| if (field.Name.Contains("UnusedStringParameter")) continue; | ||
|
|
||
| sb.AppendLine($"public object? Parameter{i}"); |
There was a problem hiding this comment.
Let's add public Type? Parameter1Type here as well
| string? IConditionStringParameterGetter.FirstStringParameter => throw new NotImplementedException(); | ||
| string? IConditionStringParameterGetter.SecondStringParameter => throw new NotImplementedException(); | ||
| Condition.Function IConditionDataGetter.Function => throw new NotImplementedException(); | ||
| public object? Parameter1 |
There was a problem hiding this comment.
Defining these like this makes it harder for the implementing classes to implement the actual setups.
In theory, these could be virtual, which would mean the inheriting classes would need to override.
that's one way.
The way I've typically been doing it is writing them like the lines above, which specifically associate them with the interface call, rather than being straight fields on the class
I think this allows the inheriting classes to define the fields for real themselves for their implementation of the interface.
There was a problem hiding this comment.
I implemented the interfaces explicitly. I think it might be good if we didn't implement the properties explicitly in ConditionData so we enforce that concrete types implement it (we could make them abstract for example).
There was a problem hiding this comment.
think it might be good if we didn't implement the properties explicitly in ConditionData
agreed!
| { | ||
| try | ||
| { | ||
| conditionData.Parameter1 = value; |
There was a problem hiding this comment.
I think ideally it wouldn't need to throw and catch to detect a false (exceptions are a decent overhead). In theory we could do the same check we do in the parameter set, and just return false
if (value is not IFormLinkOrIndex<IKeywordGetter>) return false;
Would be a hyperoptimization.. pretty low prio
There was a problem hiding this comment.
That would need to be implemented for every concrete condition data though right?
2 participants
Generate Parameter1 and Parameter2 properties to enable access to parameters without knowing the concrete condition data type