Fix part of #209: Introduce a tokenizer and parser for NumberWithUnits#6137
Open
theMr17 wants to merge 84 commits into
Open
Fix part of #209: Introduce a tokenizer and parser for NumberWithUnits#6137theMr17 wants to merge 84 commits into
theMr17 wants to merge 84 commits into
Conversation
… feat/number-with-units-parser
theMr17
removed
the
stale
BenHenning
requested changes
Jul 2, 2026
BenHenning
left a comment
BenHenning
left a comment
Member
There was a problem hiding this comment.
Thanks @theMr17! Really sorry about the delay. I was hoping to get this wrapped up before starting traveling, but I didn't and traveling + a conference/work collaboration + GSoC catch-up has used up all of my time until now. I should be able to review subsequent changes quickly now, so hopefully we can get this wrapped up soon.
The PR looks fantastic. I just had a few comments, and there's one open discussion about precedence that needs to be addressed. I also will see if code rabbit can do a review pass since it would be nice to get an LLM to validate the grammar implementation more concretely, though I have no clue if it will do a good job.
| fun testParser_compoundUnit_withExponent_returnsTrailingTokensError() { | ||
| // "Nm^2" is ambiguous (N*m^2 vs (N*m)^2), so the parser should fail. | ||
| // Decomposition produces [N, m], then "^2" remains unconsumed. | ||
| val error = parseNumberWithUnitsExpectingFailure("5 Nm^2") |
Member
There was a problem hiding this comment.
This case might lead to user confusion. How does mathjs handle it?
Member
Author
There was a problem hiding this comment.
The test is now updated, based on the other comment related to this.
Member
Author
There was a problem hiding this comment.
However, mathjs does not support units without spaces between them.
| // "N m" (space-separated) should produce the same result as "Nm" (compound). | ||
| val spacedResult = parseNumberWithUnitsExpectingSuccess("50 N m") | ||
| val compoundResult = parseNumberWithUnitsExpectingSuccess("50 Nm") | ||
| assertThat(compoundResult).apply { |
Member
There was a problem hiding this comment.
You could also use proto lite subject's is equal to for these types of tests, I think, for confidence that the results are entirely and deeply equal.
| fun testParser_compoundUnit_negativeNumber_parsesCorrectly() { | ||
| val result = parseNumberWithUnitsExpectingSuccess("-5 Nm") | ||
| assertThat(result).apply { | ||
| hasRealValueThat().isEqualTo(-5.0) |
Member
There was a problem hiding this comment.
Everywhere here & in the tokenizer tests: should we consistently be using isWithin-esque assertions for real numbers to avoid skew?
Member
Author
There was a problem hiding this comment.
Updated these checks to allow a tolerance of up to 1e-5.
| } | ||
|
|
||
| @Test | ||
| fun testParser_compoundUnit_wattSecond_spacedEquivalent_matchesCompound() { |
Member
There was a problem hiding this comment.
Ditto here & for other equivalence checks.
|
|
||
| @Test | ||
| fun testParser_dollarPrefix_negativeFraction_parsesCorrectly() { | ||
| val result = parseNumberWithUnitsExpectingSuccess("$-1/4") |
Member
There was a problem hiding this comment.
Does the other direction parse, too (negative before currency)?
Member
Author
There was a problem hiding this comment.
No it doesn't parse the other way. Do we need to support that?
| } | ||
|
|
||
| @Test | ||
| fun testParser_paiseCapitalized_parsesCorrectly() { |
Member
There was a problem hiding this comment.
Here & elsewhere: you have lots of groups of tests doing the same thing with just the value deviating (or can be made to just be the unit in most cases). Not a hard requirement, but it could be nice to leverage parameterized tests for these cases. It would pretty substantially reduce the length of the test file without compromising readability. However, I consider it optional since a test purist argument might be to never use parameterized tests, anyway, and it's extra work for minimal benefit (test maintenance and auditing, though I suspect we'll rarely do this since hopefully the parser rarely needs to be updated).
Edit: if you decide to act on this feedback, please do so in a separate commit after all the other changes are addressed so that I can review it separately since it will otherwise hide other test changes.
Member
Author
There was a problem hiding this comment.
Thanks for the suggestion! I agree it's worth it for the file size reduction. I've updated the file to use OppiaParameterizedTestRunner for the repetitive test groups (abbreviations, spelled-out forms, case variants, and SI prefixes).
This compressed 118 individual tests into 4 parameterized methods, saving almost 1400 lines of code. The total test execution count remains exactly the same.
For you reference: dc26ae2
| } | ||
|
|
||
| @Test | ||
| fun testParser_divisionWithNoNumeratorUnit_returnsUnitExpectedError() { |
Member
There was a problem hiding this comment.
Test name doesn't match error tpye in outcome.
Member
Author
There was a problem hiding this comment.
Updated this and an another test which had a naming inconsistency.
| * an unrecognized unit, attempts to decompose the token into multiple known units. | ||
| */ | ||
| private fun parseUnitsFromNextToken(): NumberWithUnitsParsingResult<List<NumberUnitExpression>> { | ||
| val singleResult = parseUnitWithExponent() |
Member
There was a problem hiding this comment.
How does mathjs handle this? I would imagine we should resolve to something predictable and be consistent about that since this doesn't seem like an actual error situation. However, I think I lean toward Nm^2 being N*m^2 since that seems a bit closer to cases like m/Ns^2 where the ^2 attaches just to the second but both the Newton and the second are divided and thus get negative exponents (though I see the case here that this may be the same problem, anyway, so this may be circular reasoning).
I asked Gemini and it agrees with my preference but for a better and different reason: we should follow PEMDAS rules for operator precedence here with multiplications and left-hand associativity (or right-hand in the case expontentiation).
Edit: Here's my Gemini conversation. It suggests the grammar is correct but the parser may not have implemented it correctly, hence the discrepancy. See: https://share.gemini.google/zGXS1nGTUzy1.
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a "number with units" feature: new protobuf messages defining expressions with numeric values and units, a tokenizer converting raw strings into typed tokens, a parser converting tokens into structured proto expressions with detailed error types, Truth test subjects for assertions, and associated Bazel targets, test exemptions, and extensive tokenizer tests. ChangesNumber-with-units support
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NumberWithUnitsParser
participant NumberWithUnitsTokenizer
participant NumberWithUnitsParsingError
Caller->>NumberWithUnitsParser: parseNumberWithUnits(rawExpression)
NumberWithUnitsParser->>NumberWithUnitsTokenizer: tokenize(normalized input)
NumberWithUnitsTokenizer-->>NumberWithUnitsParser: Sequence of Token
NumberWithUnitsParser->>NumberWithUnitsParser: parse number and compound unit
alt parse fails
NumberWithUnitsParser->>NumberWithUnitsParsingError: build structured error
NumberWithUnitsParser-->>Caller: Failure(error)
else parse succeeds
NumberWithUnitsParser-->>Caller: Success(NumberWithUnitsExpression)
end
Related issues: None referenced in the provided diff. Related PRs: None referenced in the provided diff. Suggested labels: feature, math, tests Suggested reviewers: None specified. 🐰 A number hops in, a unit close behind, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@testing/src/main/java/org/oppia/android/testing/math/NumberWithUnitsSubject.kt`:
- Around line 73-87: Update hasPrefixThat() in NumberWithUnitsSubject so it is
parameterized like hasSuffixWithIndexThat(index) and can assert against any
prefix unit instead of always returning units.first(). Keep the existing unit
selection logic for actual.expressionFormatCase, then use the provided index to
pick the prefix unit and validate bounds appropriately with a clear failure
message. Ensure the new API stays symmetric with the suffix helper and still
returns a NumberUnitSubject for the selected unit.
In
`@testing/src/main/java/org/oppia/android/testing/math/NumberWithUnitsTokenSubject.kt`:
- Around line 17-20: The primary constructor on NumberWithUnitsTokenSubject is
public, which is inconsistent with Truth subject conventions. Make the
constructor private so only the subject factory/assertThat path can create
instances, matching the pattern already used by NumberWithUnitsSubject and
keeping construction centralized within NumberWithUnitsTokenSubject.
In `@utility/src/main/java/org/oppia/android/util/math/NumberWithUnitsParser.kt`:
- Around line 99-181: The bare minus handling in parseNumber() is returning the
currency-specific NumberExpectedAfterCurrencyPrefixError even when the parser is
being used from parseSuffixFormattedValue() with no currency prefix. Update
parseNumber() to accept a call-context flag (for example, whether it follows a
currency prefix) and use it to choose the correct error when a minus sign is not
followed by a number; then update parsePrefixFormattedValue() to pass the
currency-prefix context while parseSuffixFormattedValue() continues to use the
non-currency path.
- Around line 268-321: `parseUnitsFromNextToken()` is incorrectly falling back
to compound-unit decomposition after `parseUnitWithExponent()` has already
consumed input and failed, which can hide `MissingExponentError`. In
`NumberWithUnitsParser`, snapshot `tokens.getRetrievalCount()` before calling
`parseUnitWithExponent()`, and only call `tryDecomposeCompoundUnit()` when the
parse failed without advancing tokens; otherwise return the original failure so
`kg^`-style inputs are not reinterpreted as a fresh unit.
In
`@utility/src/main/java/org/oppia/android/util/math/NumberWithUnitsTokenizer.kt`:
- Around line 70-78: The KDoc for NumberWithUnitsTokenizer.tokenizeNumber
currently claims whitespace is allowed between digits, but the implementation
only supports whitespace around the decimal point and not inside an integer
digit run. Update the documentation on tokenizeNumber and any related Kdoc near
parseInteger to accurately describe the supported number formats, and remove
wording that implies inputs like "1 2 cm" are accepted.
- Around line 5-13: Update the KDoc on NumberWithUnitsTokenizer and tokenizeUnit
to describe the current behavior: this class only lexes raw unit text by
consuming contiguous VALID_UNIT_CHARACTERS and does not interpret
singular/plural forms, abbreviations, or naming conventions. Remove the outdated
semantic wording from the class-level docs and any related function docs, and
make sure the comments clearly point to NumberWithUnitsParser as the place where
unit normalization and interpretation now happen.
- Around line 264-280: Remove the unused private helper expectNextCharsForUnit
from NumberWithUnitsTokenizer, since it is not referenced anywhere and was
superseded by the Token.Unit raw-capture flow. Delete the helper and any
now-unneeded related logic or comments, and keep the tokenizer behavior centered
on the existing peek/consume parsing helpers and Token.Unit handling.
In
`@utility/src/test/java/org/oppia/android/util/math/NumberWithUnitsTokenizerTest.kt`:
- Around line 1912-1921: Strengthen the
`testTokenize_unknownSymbols_parsesCorrectly` coverage in
`NumberWithUnitsTokenizerTest` because the `"10 °C"` iteration can currently
pass without verifying the trailing `C` token. Update the assertions around
`NumberWithUnitsTokenizer.tokenize(input)` to check the full token count and
explicitly validate the third token for the `"10 °C"` case, while keeping the
existing `tokens[0]` and `tokens[1]` checks. Use the existing
`NumberWithUnitsTokenizer` and `Token` assertions in this test to confirm the
tokenizer handles valid unit-starting characters after an invalid symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0da1c79a-27d1-4c60-ba2f-e5559066c667
📒 Files selected for processing (13)
model/src/main/proto/interaction_object.protoscripts/assets/file_content_validation_checks.textprotoscripts/assets/test_file_exemptions.textprototesting/src/main/java/org/oppia/android/testing/math/BUILD.bazeltesting/src/main/java/org/oppia/android/testing/math/NumberWithUnitsSubject.kttesting/src/main/java/org/oppia/android/testing/math/NumberWithUnitsTokenSubject.ktutility/src/main/java/org/oppia/android/util/math/BUILD.bazelutility/src/main/java/org/oppia/android/util/math/NumberWithUnitsParser.ktutility/src/main/java/org/oppia/android/util/math/NumberWithUnitsParsingError.ktutility/src/main/java/org/oppia/android/util/math/NumberWithUnitsTokenizer.ktutility/src/test/java/org/oppia/android/util/math/BUILD.bazelutility/src/test/java/org/oppia/android/util/math/NumberWithUnitsParserTest.ktutility/src/test/java/org/oppia/android/util/math/NumberWithUnitsTokenizerTest.kt
Comment on lines
+264
to
+280
| /** | ||
| * Verifies that the next characters in the iterator exactly match the expected string. | ||
| * | ||
| * This method is used to validate unit names by checking that the remaining characters | ||
| * in the iterator match the expected unit suffix. It consumes the characters if they match. | ||
| * | ||
| * @param chars the expected string to match | ||
| * @return true if all characters match exactly, false otherwise | ||
| */ | ||
| private fun PeekableIterator<Char>.expectNextCharsForUnit( | ||
| chars: String | ||
| ): Boolean { | ||
| for (c in chars) { | ||
| expectNextValue { c } ?: return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Unused private helper expectNextCharsForUnit.
This function is not referenced anywhere in the file. It appears to be a leftover from the earlier per-unit character-matching design, superseded by the generic Token.Unit raw-capture approach adopted after review discussion. Recommend removing it.
🧹 Suggested removal
- /**
- * Verifies that the next characters in the iterator exactly match the expected string.
- *
- * This method is used to validate unit names by checking that the remaining characters
- * in the iterator match the expected unit suffix. It consumes the characters if they match.
- *
- * `@param` chars the expected string to match
- * `@return` true if all characters match exactly, false otherwise
- */
- private fun PeekableIterator<Char>.expectNextCharsForUnit(
- chars: String
- ): Boolean {
- for (c in chars) {
- expectNextValue { c } ?: return false
- }
- return true
- }
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Verifies that the next characters in the iterator exactly match the expected string. | |
| * | |
| * This method is used to validate unit names by checking that the remaining characters | |
| * in the iterator match the expected unit suffix. It consumes the characters if they match. | |
| * | |
| * @param chars the expected string to match | |
| * @return true if all characters match exactly, false otherwise | |
| */ | |
| private fun PeekableIterator<Char>.expectNextCharsForUnit( | |
| chars: String | |
| ): Boolean { | |
| for (c in chars) { | |
| expectNextValue { c } ?: return false | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@utility/src/main/java/org/oppia/android/util/math/NumberWithUnitsTokenizer.kt`
around lines 264 - 280, Remove the unused private helper expectNextCharsForUnit
from NumberWithUnitsTokenizer, since it is not referenced anywhere and was
superseded by the Token.Unit raw-capture flow. Delete the helper and any
now-unneeded related logic or comments, and keep the tokenizer behavior centered
on the existing peek/consume parsing helpers and Token.Unit handling.
Member
Author
There was a problem hiding this comment.
The unused function is now removed.
There was a problem hiding this comment.
@theMr17, thanks for addressing this—removing the obsolete helper keeps the tokenizer focused on the current raw unit-token flow.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Comment on lines
+1912
to
+1921
| @Test | ||
| @Iteration("10 °C", "input=10 °C") | ||
| @Iteration("10 °", "input=10 °") | ||
| @Iteration("10 Ω", "input=10 Ω") | ||
| fun testTokenize_unknownSymbols_parsesCorrectly() { | ||
| val tokens = NumberWithUnitsTokenizer.tokenize(input).toList() | ||
|
|
||
| assertThat(tokens[0]).isPositiveIntegerWithValueThat().isEqualTo(10) | ||
| assertThat(tokens[1]).isInvalidToken() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing full-token assertions for the "10 °C" case.
For "10 °C", after the invalid ° character, C is a valid unit-starting letter, so the tokenizer will actually emit a third token (Token.Unit("C")). Unlike every other test in this file, this test never asserts tokens.hasSize(...) or inspects tokens[2], so a regression in how trailing valid characters are handled after an invalid symbol would go undetected.
✅ Suggested strengthened assertions
fun testTokenize_unknownSymbols_parsesCorrectly() {
val tokens = NumberWithUnitsTokenizer.tokenize(input).toList()
assertThat(tokens[0]).isPositiveIntegerWithValueThat().isEqualTo(10)
assertThat(tokens[1]).isInvalidToken()
+ // Note: "10 °C" produces a trailing Unit("C") token since 'C' is a valid unit-starting
+ // letter; other iterations (e.g. "10 °") end after the invalid token.
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@utility/src/test/java/org/oppia/android/util/math/NumberWithUnitsTokenizerTest.kt`
around lines 1912 - 1921, Strengthen the
`testTokenize_unknownSymbols_parsesCorrectly` coverage in
`NumberWithUnitsTokenizerTest` because the `"10 °C"` iteration can currently
pass without verifying the trailing `C` token. Update the assertions around
`NumberWithUnitsTokenizer.tokenize(input)` to check the full token count and
explicitly validate the third token for the `"10 °C"` case, while keeping the
existing `tokens[0]` and `tokens[1]` checks. Use the existing
`NumberWithUnitsTokenizer` and `Token` assertions in this test to confirm the
tokenizer handles valid unit-starting characters after an invalid symbol.
Member
Author
There was a problem hiding this comment.
This seems to be unnecessary.
There was a problem hiding this comment.
@theMr17, agreed—this is unnecessary for the behavior under test. I’m withdrawing the suggestion.
|
Hi @theMr17, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
Bot
added
stale
Contributor
Coverage ReportResultsNumber of files assessed: 305 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
Contributor
Coverage ReportResultsNumber of files assessed: 286 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
…sing consumes no tokens and add tests
…on of concerns between lexing and parsing
… for consolidated unit test assertions
Member
Author
|
Thanks for the review @BenHenning! I have addressed all the comments in your review as well as Rabbit's review. PTAL! |
|
Hi @theMr17, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
Bot
added
the
stale
theMr17
removed
the
stale
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Explanation
Fixes part of #209
This PR introduces a
NumberWithUnitsTokenizerthat converts raw string input into aSequence<Token>. It supports numeric literals, operators/symbols (-, /, ^, *, parentheses), currency symbols/prefixes/suffixes, SI prefixes, and a broad set of physical units. The tokenizer is implemented with the existingPeekableIterator. This keeps behavior aligned with LL(1) parsing constraints and avoids unnecessary processing when parsing fails early.Further the
Sequence<Token>is passed to theNumberWithUnitsParser, which is responsible for enforcing the grammar rules as mentioned here. Error handling of the parser is explicit and non-exception based viaNumberWithUnitsParsingResult.Success/Failure, with parser failures mapped to domain-specificNumberWithUnitsParsingErrorvalues (e.g. missing exponent/denominator/unit, unbalanced parentheses, invalid tokens, trailing tokens)A tokenizer Truth subject:
NumberWithUnitsTokenSubjectis added that provides typed assertion helpers for every relevant token shape (numeric tokens, symbols, units, invalid tokens). This reduces repetitive casting and improves readability in tokenizer tests. And aNumberWithUnitsSubjectwith helpers for asserting numeric representation (real vs fraction) and unit structure (count, names, name-based unit lookup, exponent assertions). Both custom subject files include TODOs to add dedicated tests for the subjects themselves (#6151). These helpers are straightforward wrappers around type checks and proto field assertions; follow-up dedicated subject tests are tracked separately to keep PR scope manageable.Essential Checklist
Summary by CodeRabbit
New Features
Bug Fixes