iTwin
Quantity parser returns `NaN` instead of an Error in some edge cases.
When parsing two dots .. or two comas ,, the length formatter return NaN instead of an error.
I am parsing using the engineering length parser, in imperial.
I suggest that whenever we parse a NaN value, we convert the result to an error instead.
By error, do you mean returning a ParseError? I agree, for parseRatioFormat this is already what we do. We should streamline the behavior across the code.
We also still have the split between throwing a QuantityError and returning (not throwing) a ParseError. The reasoning there is: QuanityError is for unexpected things, like misconfiguration or broken metadata while ParseError is for expected things (we expect parsing to fail frequently with unvalidated inputs). I think keeping it like that makes sense.
Hi,
Im not sure which parsing got used here. Currently there are 2 parsing methods in Parser.ts, the async parseIntoQuantity and the sync parseQuantityString.
they both use Parser.parseQuantitySpecification to parse token and there was an error in there handling dots which I addressed in the pr. Now there shouldnt be any NaN value being returned.
The problem with returning an error on async parseIntoQuantity is that it returns a QuantityProps
export interface QuantityProps {
readonly magnitude: number;
readonly unit: UnitProps;
readonly isValid: boolean;
}
it doesnt contain info on what the ParseError is. It does tell whether the returned Quantity is valid. (it did in the call stack calls getQuantityValueFromParseTokens but it didnt return the ParseQuantityResult returned right here
Maybe we can change the return type of the async method to match with the sync one. The sync one returns a QuantityParseResult
export type QuantityParseResult = ParsedQuantity | ParseQuantityError;
export interface ParsedQuantity {
ok: true;
value: number; // the magnitude
}
export interface ParseQuantityError {
ok: false;
error: ParseError;
}
After the fix in the pr, the sync method returns ParseError.NoValueOrUnitFoundInString when parsing strings with only dots or comas.
The async method just returns an empty quantity right now, but it seemed more reasonable to me that the async method should return the same parse result as the sync one.
Im just not sure how this might affect things cause its changing the return type of a public api.
Note, I'm not 100% certain that it returns NaN. The AccuDraw input fields displays NaN.
When an error is returned, the AccuDraw input field handles it correctly, it ignores the value and uses another valid one.
When typing two dots .. something else unexpected occurs because NaN is displayed.
I'd like more information on how AccuDraw's input field handles bad inputs returned from the quantity parser. As mentioned by @naronchen , for our synchronous parsing we return ParseError.NoValueOrUnitFoundInString. It's more likely a change, if we do want to make a change, would lie within accudraw's handling of quantity parsing errors
