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

Some improvements for next major release #21

Open
wants to merge 9 commits into
base: next
Choose a base branch
from

Conversation

vansari
Copy link

@vansari vansari commented Dec 28, 2022

I have seen your Repo and find it very useful. So I want to contribute some useful changes.

Changes:

  • declare strict types
  • Added Return Types
  • Remove unnecessary consts
  • use regex group name for more readability
  • cast variables to string
  • fixed compatibility for PHP < 8.0 with substr in strict mode

PR Description

Done some code styles fixes and a bugfix with PHP < 8.0 in strict type mode

Checklist

  • I agree with the Code Contribution License Agreement in CONTRIBUTING.md

- Added Return Types
- Remove unnecessary consts
- use regex group name for more readability
- cast variables to string
- fixed compatibility for PHP < 8.0 with substr
@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Merging #21 (3d9206d) into next (8f0425c) will increase coverage by 0.66%.
The diff coverage is 94.73%.

❗ Current head 3d9206d differs from pull request most recent head 72eaa9d. Consider uploading reports for the commit 72eaa9d to get more accurate results

@@             Coverage Diff              @@
##               next      #21      +/-   ##
============================================
+ Coverage     94.11%   94.78%   +0.66%     
- Complexity      103      104       +1     
============================================
  Files             1        1              
  Lines           221      230       +9     
============================================
+ Hits            208      218      +10     
+ Misses           13       12       -1     
Impacted Files Coverage Δ
src/Decimal.php 94.78% <94.73%> (+0.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

- revert signature change for resultScale
- revert deletion of deprecated function
@dereuromark
Copy link
Contributor

dereuromark commented Dec 28, 2022

Thanks a lot for your contribution

Lets separate those into two different things:

  • Bugfixes and BC improvements: master
  • BC breaking changes and remove of deprecations etc: next

The next branch will then eventually become a new major version in the future.

I added this to the docs: https://github.com/spryker/decimal-object/blob/master/docs/README.md#contributing

@vansari vansari changed the base branch from master to next December 28, 2022 20:54
@vansari
Copy link
Author

vansari commented Dec 28, 2022

@dereuromark So I have changed the target to next. I think this changes would be great :-)

@vansari vansari changed the title Some Code Fixes Some improvements for next major release Dec 28, 2022
@dereuromark
Copy link
Contributor

Cool
But those string casts etc would be useful to have already for master, right?
You might want to extract those first into master, and then we can merge master + your additional changes into next.

@vansari
Copy link
Author

vansari commented Jan 2, 2023

I have updated the code. So the next release will support only PHP Versions > 8.0

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

Successfully merging this pull request may close these issues.

2 participants