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

Support for BcMath\Number objects #121

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

derrabus
Copy link
Contributor

@derrabus derrabus commented Jan 6, 2025

This PR adds support for the new BcMath\Number value objects (php/php-src#13741). It allows comparing two numbers as well as comparing a number to an integer or a numeric string. The goal is to make the following assertions pass in PHPUnit:

self::assertEquals(new Number('13.37'), new Number('13.370000'));
self::assertEquals('13.37', new Number('13.37'));
self::assertEquals(13, new Number('13.37'));

The PR also allows to compare to numbers with a delta.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.04%. Comparing base (07ba41d) to head (dcff245).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #121      +/-   ##
============================================
+ Coverage     99.00%   99.04%   +0.04%     
- Complexity      142      159      +17     
============================================
  Files            15       16       +1     
  Lines           402      421      +19     
============================================
+ Hits            398      417      +19     
  Misses            4        4              

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

@@ -1,5 +1,10 @@
parameters:
level: 10
phpVersion: 80402
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this setting, PHPStan would report BcMath\Number as an unknown class and we would lose type coverage for the new NumberComparator class.

Copy link
Owner

Choose a reason for hiding this comment

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

I just updated PHPStan to version 2.1.1 on main. Maybe that solves this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. PHPStan 2.0.1 does already know about the Number class, so upgrading to 2.1.1 doesn't change anything for me.

The problem is that PHPStan needs a target PHP version for its analysis. If we don't set it here, it will guess it from the composer.json file and assume PHP 8.2.0 because of this setting:

comparator/composer.json

Lines 41 to 43 in 07ba41d

"platform": {
"php": "8.2.0"
},

The Number class simply does not exist on PHP 8.2 yet, so PHPStan technically correct when complaining about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

BC Number support is tracked here: phpstan/phpstan#12099

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staabm That's for the overloaded operators only which is a different issue. Once the issue you've linked is resolved, I can remove the one ignored error from the PHPStan configuration file.

@sebastianbergmann sebastianbergmann changed the title Support for BCMath\Number objects Support for BCMath\Number objects Jan 6, 2025
@derrabus derrabus changed the title Support for BCMath\Number objects Support for BcMath\Number objects Jan 6, 2025
@derrabus
Copy link
Contributor Author

derrabus commented Jan 6, 2025

I've rebased the PR to make the CI run with the updated tools.

@sebastianbergmann sebastianbergmann merged commit 22506df into sebastianbergmann:main Jan 6, 2025
8 checks passed
@derrabus derrabus deleted the feature/number branch January 6, 2025 10:09
@sebastianbergmann
Copy link
Owner

@derrabus Thank you! Do you think sebastian/exporter needs to learn about BcMath\Number as well?

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

Successfully merging this pull request may close these issues.

3 participants