Skip to content

Factor unit inversion in class + in parser#29

Open
zoltan-vespertool wants to merge 2 commits into
masterfrom
parser-inverts-everything-in-the-denominator
Open

Factor unit inversion in class + in parser#29
zoltan-vespertool wants to merge 2 commits into
masterfrom
parser-inverts-everything-in-the-denominator

Conversation

@zoltan-vespertool

Copy link
Copy Markdown

No description provided.

Copilot AI review requested due to automatic review settings May 12, 2026 14:15
@zoltan-vespertool zoltan-vespertool changed the title Parser inverts everything in the denominator Factor unit inversion in class + in parser May 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request updates unit parsing and inversion behavior so dimensionless factor parts (e.g., SI prefixes like k) correctly participate in exponentiation and denominator inversion, and adds tests around invert() and factor/unit interactions.

Changes:

  • Update Parser so FactorUnitPart ratios are raised to the parsed unit power (including negative powers/denominators).
  • Add FactorUnitPart::invert() to invert via reciprocal ratio (since factor parts always have power 1).
  • Extend test coverage for Unit::invert(), factor inversion, and parsing cases like km^2 and factor units in denominators.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/Parser.php Adjusts how parsed powers are applied to factor parts during expansion of registered units.
src/FactorUnitPart.php Adds an override for invert() that reciprocates the factor ratio.
tests/UnitTest.php Adds tests validating Unit::invert() behavior with factor + powered dimension parts.
tests/ParserTest.php Updates expectations around powered/denominator parsing and adds new parsing tests.
tests/FactorUnitPartTest.php Adds a unit test for FactorUnitPart::invert().
Comments suppressed due to low confidence (1)

src/Parser.php:49

  • When recreating UnitPart instances during parsing, the original offset is dropped (UnitPart::$offset defaults to 0). This breaks parsing of offset units (e.g., celsius/fahrenheit from RegistryBuilder) when they appear in expressions or with powers, because the resulting Unit loses its offset and conversions will be incorrect. Pass through $part->getOffset() when constructing the new UnitPart.
            $parts[] = array_map(function (UnitPart|FactorUnitPart $part) use ($power) {
                if ($part instanceof FactorUnitPart) {
                    return new FactorUnitPart($part->getRatio() ** $power);
                }

                return new UnitPart(
                    $part->getRatio(),
                    $part->getDimension(),
                    $part->getPower() * $power,
                );

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/ParserTest.php
Comment on lines +255 to +266
public function test_parses_km_squared()
{
$result = $this->parser->parse('km^2');

$this->assertCount(2, $result->getParts());

[$k, $m] = $result->getParts();

$this->assertEquals(null, $k->getDimension());
$this->assertEquals(1000 ** 2, $k->getRatio());
$this->assertEquals(1, $k->getPower());

Comment thread tests/UnitTest.php
Comment on lines +197 to +198
$this->assertEquals($invertedTwice->getRatio(), $hundredSquareMeters->getRatio());
$this->assertEquals($invertedTwice->getDimensions(), $hundredSquareMeters->getDimensions());

$factorInverted = $factor->invert();

$this->assertEquals(FactorUnitPart::class, get_class($factorInverted));
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.

2 participants