Closed jgersti closed 2 years ago
Great start on this and thank you so much for adding a new test file specifically for comparisons, I agree this is needed.
The painful bit here is going to be around comparison.
So right now, comparison for the equals operator is done as "a.Compare(b) == 0", not unreasonably.
The Compare() functions needs some work, I think.
Currently it performs the COMPARE operation on the given types, casts to a number, and returns it.
I think we simply need to fix this.
Calling COMPARE is still fine, I think. But we need to look at the result. If the result is a boolean (returnType=Return.Bool) return 0 if true, -1 otherwise?. If numeric, return the numeric directly?
That'll get rid of the bad Numeric->Bool hack that's in place.
Thoughts?
I would leave the comparison stuff for later. The main concern should be the parsing errors.
Is there way to reasonably way to step through the parser to see where the cast branch is not matched/taken ?
I'm not sure you'll be able to put all of the cast functions in the right place without fixing comparison, but dig through it and see what you find.
As for walking through, you can debug the tests to see what's happening. I had to re-add the debug configurations to get it to work though (shouldn't have gotten rid of them previously). Re-added in the #135 PR. Feel free to take a look. I will likely push them soon.
A good point to put a breakpoint would be the Cast
All cast to color
, list
and number
are broken on a fundamental level.
The token color
is matched as a PropertyCommandParameter
, the tokens list
and number
as PropertyAggregationCommandParameter
, but the rule processor tries to match a VariableCommandParameter
.
PropertyAggregationCommandParameter
s also cannot be distinguished.
The only way to i see to fix this is something along the way of #132.
The cast operation is intended to use a string as the 2nd operand. I agree it'd be nicer if you didn't have to surround in quotes.
I suggest we get the cast operations in a good state, using "number", "list", "color" and then can look at how we might extend this to avoid needing double quotes.
The idea of "this word might mean 2 things" could be generalized so we don't have to create classes for each no ambiguous items, if we can find a common place in the parsing tree to put the branching. But let's solve that as a follow up?
Can you take a look at the way the ambigious tokens are handled. Unfortunatly it is rather spedific to the casts.
The only test still failing is the expected bool -> number
one with the changed behaviour
I like where you are going with this, but I agree it's a little specific to casting. Let me take a thorough look and propose something a bit more generic.
My gut says something like:
AddAmbiguousPropertyWords(Words("color"), new PropertyCommandParameter(Property.Color));
We can create a new AmbiguousPropertyCommandParameter that has a CommandParameter attribute.
AddAmbiguousPropertyWords creates an AmbiguousStringCommandParameter whose sub CommandParameter attribute is the one you defined.
The parameter processor (which will need to be moved above operations I think) can then look for AmbiguousStringCommandParameter and do the branching.
With this approach you shouldn't need the alternative words mapping, I believe
Thoughts?
Take a look please if that was what you had in mind.
It should atleast be cleaner and more extensible than the previous approach.
This PR is a starting point to fix #121.
number as bool
the 2 variants of thenumber as bool
test are mutually exclusive!color as vector
One of the casts in the sequenceVector3 -> Object -> Vector3D
throws the exceptionedit: