[pradeeppeddineni]

Fixes #386.

The CQL spec defines mod for Quantity values (3.5 'cm' mod 3 'cm' is 0.5 'cm'), but cql-execution only handled Integer, Long and Decimal, so a Quantity mod returned a wrong value. The two spec tests for it (ModuloQuantity and Modulo10By3Quantity) were on the skip list as "Modulo not implemented for Quantity".

What I changed:

• Added a modulo method to Quantity in src/datatypes/quantity.ts, next to dividedBy and multiplyBy and following the same pattern. It returns null when the divisor is null or zero (same as the existing Decimal mod and the spec), converts compatible units with normalizeUnitsWhenPossible (so 1 'm' mod 30 'cm' is 10 'cm'), returns null when the units still don't match after that, and returns null on overflow like the other Quantity operations. The result keeps the unit of the operands.
• Modulo.exec in src/elm/arithmetic.ts now calls it when either operand is a Quantity. A plain number on either side is treated as a Quantity with unit '1', the same way multiplyBy and dividedBy do it.
• Removed the two entries from test/spec-tests/skip-list.txt (and the section header, which was left empty) and regenerated the spec test data with npm run build:spec-test-data. Only CqlArithmeticFunctionsTest.cql and CqlArithmeticFunctionsTest.json changed.
• Added unit tests in test/datatypes/quantity-test.ts for the same unit case, a unit conversion case, a zero or null divisor, and units that can't be converted.

This follows the approach suggested in the issue.

Local results:

• npm test: 5456 passing, 97 pending (master: 5450 passing, 99 pending). ModuloQuantity and Modulo10By3Quantity now run and pass.
• npm run lint, npm run prettier, npm run test:typecheck and tsc --noEmit are clean.

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

Submitter:

• This pull request describes why these changes were made
• Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
• Tests are included and test edge cases
• Tests have been run locally and pass
• Code coverage has not gone down and all code touched or added is covered.
• Code passes lint and prettier (hint: use npm run check to run tests, lint, and prettier)
• All dependent libraries are appropriately updated or have a corresponding PR related to this change
  Reviewer:

Name:

• Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
• The tests appropriately test the new code, including edge cases
• You have tried to break the code

[bryantaustin13]

One case this doesn't handle yet: a unitless divisor. 10.0 'g' mod 3 returns null, because the 3 becomes 3 '1' and 'g' and '1' don't normalize to the same unit.

FHIR-59659 (div and mod result units) says the remainder has "the same units as its operands after converting to common units in the same way as for division". A '1' divisor has no dimension to convert, and division keeps the dividend's unit (10.0 'g' / 3 is 3.33 'g'), so the remainder should be 1.0 'g'. The Java engine already returns that. The test is Modulo10GBy3Quantity in cqframework/cql-tests#167, which also updates the mixed-unit Modulo tests to expect 10.0 'cm' and 100.0 'g'. Those already pass with this PR, since normalizeUnitsWhenPossible converts to the smaller unit.

Suggested change in src/datatypes/quantity.ts:

Suggested change

	// The remainder only makes sense when both sides end up in the same unit
	if (unit1 !== unit2) {
	return null;
	}
	// The remainder only makes sense when both sides end up in the same unit, except that a
	// unitless divisor has no dimension to convert, so the remainder keeps the dividend's unit
	// (10 'g' mod 3 is 1 'g', as 10 'g' / 3 is 3.33 'g')
	if (unit1 !== unit2 && unit2 !== '1') {
	return null;
	}

And a test for test/datatypes/quantity-test.ts:

  it('should keep the unit of the dividend for modulo by a unitless value', () => {
    const result = new Quantity(10.0, 'g').modulo(3);
    result.unit.should.equal('g');
    result.value.should.equalDecimal(1);
    new Quantity(10.0, 'g')
      .modulo(new Quantity(3, '1'))
      .equals(new Quantity(1, 'g'))
      .should.be.true();
  });

With both on top of this branch, npm test gives 5457 passing and 97 pending, one more passing than the branch alone. npm run lint, npm run prettier and tsc --noEmit are clean.

[pradeeppeddineni]

Applied your change so a unitless divisor keeps the dividend's unit, and added the 10.0 'g' mod 3 test. Suite is green, 5457 passing.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.17%. Comparing base (e8096c6) to head (deb7b58).

Files with missing lines	Patch %	Lines
src/datatypes/quantity.ts	84.61%	2 Missing ⚠️
src/elm/arithmetic.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   89.19%   89.17%   -0.03%     
==========================================
  Files          60       60              
  Lines        5064     5080      +16     
  Branches     1446     1453       +7     
==========================================
+ Hits         4517     4530      +13     
- Misses        315      317       +2     
- Partials      232      233       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dehall]

Thank you for the PR, this looks good to me.

In reviewing I did discover two issues (#399 and #400) but since those already existed in the other arithmetic operations, I don't think they need to be specifically addressed here.