refactor: Handle percent parsing by making it a Unit #3590
+338
−161
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR represents an alternative to #3573 for solving the persistent issues with parsing
%in the mathjs expression language. I think both should be evaluated before either is merged, and of course at most one or the other should be merged, not both.My take on the characteristics of the two PRs:
#3573
this PR
isPercentagefield of OperatorNode altogether, moving it closer to FunctionNode as desired.10% + 20 % 3(namely 10 mod +20 mod 3)20% 3 + 10 %(namely 20 mod 3 plus 10 percent)50 + (20% + 10%)(namely 50 plus 30 percent = 65)x + (20% + 10%)(namely x plus 30 percent = 1.3x)math.add(10, math.unit(17, 'percent'))to get the same effect as10 + 17%. Or in other words, the percent symbol (when not used as modulo) is treated as semantically significant, rather than as syntactic sugar. Hence parse trees are more closely aligned with the text input.math.evaluate('10%')returnsmath.unit(10, 'percent')rather than 0.1, althoughmath.number(math.evaluate('10%'))remains the same. In other words, if you coerce the results of evaluation (that might come out to a percentage) to a number, you get the same value. And the new Unit return value might be considered a feature, in that for a calculator application it might be more natural to see the result of30% + 25%as55%rather than as0.55. This last point seems like a natural way to leverage the power of Unit and the possibility it allows of dimensionless units.cube(math.unit(5, 'cm'))by some mechanism I honestly could not track down, but the modified behavior appears to be the correct one, in that the Unit class result ofu.pow(3)is explicitly marked as allowing automatic simplification, and the new behavior is precisely the result of automatic simplification (e.g0 cm^3simplifying to0 literinstead of staying0 cm^3as it did before, for reasons I can't fathom).To sum up, this PR is definitely more radical and choosing it would depend on whether some of the above characteristics are seen as positives and/or simplifications that justify the bigger change.
@josdejong will have to determine which direction to pursue. Happy to adjust this PR as desired if it happens to represent the chosen strategy.
Resolves #2861.
Resolves #3562.
Resolves #3563.