Skip to content
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

Add limitations to quantity parsing logic #7671

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

hl662
Copy link
Contributor

@hl662 hl662 commented Feb 7, 2025

Closes #7596

  • Improve tests, ensuring default parsing and formatting in core frontend's QuantityFormatter produces equivalent values between each other.
  • Update the tokenization logic, adding limits and document them in scenarios where a spacer conflicts with a math operator. In particular:
    • Mathematical operations only apply when it is in between whitespace. So -2FT 6IN + 6IN is equal to -2FT-6IN + 6IN, and -2FT-6IN - 6IN is not equal to -2FT-6IN-6IN.
    • For a value like 2FT 6IN-0.5, if the FormatProps has the spacer set to -, the - sign will be treated as a spacer and not subtraction. However, the 0.5 value will use the default unit conversion provided to the parser, because it's not a part of the composite unit when that composite is made up of only 2 units - FT and IN.

@hl662 hl662 self-assigned this Feb 7, 2025
@hl662 hl662 marked this pull request as ready for review February 10, 2025 15:47
@bbastings
Copy link
Contributor

I only approve of the test changes to make sure the parser/formatter match. Don't know enough about the parsing code and potential separators to say whether this is a good solution. Not supporting * and / makes this a big who cares for me.

@hl662
Copy link
Contributor Author

hl662 commented Feb 10, 2025

I only approve of the test changes to make sure the parser/formatter match. Don't know enough about the parsing code and potential separators to say whether this is a good solution. Not supporting * and / makes this a big who cares for me.

I think the best solution, is to take out math operations like you say and figure out a way to add math support on top of quantity, not bake it in, something similar to what you did with AccuDraw. But that shouldn't be done as part of this PR - we want to slot in an alternative the same time we drop this support. We can open an issue to address exactly this later on

Immediately? I want to address the bug as is and the lack of tests for these cases.

@aruniverse
Copy link
Member

fyi @mathieu-fournier

@bbastings
Copy link
Contributor

@hl662 So what's up with this PR, has any decision been made? Also, ideally I'd like a fix backported to 4.11.x so I can remove my hack to make AccuDraw work...

private makeParserHappy(value: string, isAngle: boolean): string {
// TODO: Work around for default length parser not accepting output formatted with dash separator, ex. 20'-6"...
const parserSpec = (isAngle ? undefined : this.getLengthParser());
if (undefined === parserSpec)
return value;
return (FormatType.Fractional === parserSpec.format.type && -1 !== value.indexOf("'-") ? value.replaceAll("'-", "':") : value);
}

Comment on lines +342 to +344
if (isCharOperator && nextCharCode !== QuantityConstants.CHAR_SPACE && prevCharCode !== QuantityConstants.CHAR_SPACE) {
// ignore spacer if it's not at the start or end, not whitespace, and is not in between whitespace
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to force a white space on either side of the operator.

If we are going to rely on spacing maybe a space before the operator is enough.

Another idea that is more complex is that we know how many segments there could be so we know the max number of separators ... if we see more it must be an operator. If we did this we would still need to do something with spaces to handle cases where a value didn't need all segments. e.g. exactly 12'.

I'd be happy to start simple and say there must be a space before if the spacer character matches the operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default imperial units length parser does not accept string from default length formatter.
7 participants