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

Refactoring #145

Closed
wants to merge 208 commits into from
Closed

Refactoring #145

wants to merge 208 commits into from

Conversation

bumbummen99
Copy link
Owner

@bumbummen99 bumbummen99 commented Jan 17, 2022

  • Raise required PHP and dependency versions according to supported versions
  • Add type hints up to 8.0
  • Support Laravel 9
  • Use MoneyPHP instead of direct values
  • Adjust the general workflow

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2022

Codecov Report

Merging #145 (b14f991) into master (82a1e7d) will decrease coverage by 5.14%.
The diff coverage is 91.56%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #145      +/-   ##
============================================
- Coverage     98.41%   93.27%   -5.15%     
+ Complexity      168      130      -38     
============================================
  Files             7        5       -2     
  Lines           442      342     -100     
============================================
- Hits            435      319     -116     
- Misses            7       23      +16     
Flag Coverage Δ
unittests 93.27% <91.56%> (-5.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/CartItemOptions.php 100.00% <ø> (ø)
src/Cart.php 91.66% <89.18%> (-7.58%) ⬇️
src/CartItem.php 97.50% <96.15%> (-1.65%) ⬇️
src/Config/cart.php 100.00% <100.00%> (ø)
src/ShoppingcartServiceProvider.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82a1e7d...b14f991. Read the comment docs.

@bumbummen99 bumbummen99 added enhancement New feature or request help wanted Extra attention is needed labels Feb 6, 2022
@bumbummen99 bumbummen99 self-assigned this Feb 6, 2022
This was referenced Feb 8, 2022
@hlorofos
Copy link

Keep going!

@Kwaadpepper
Copy link

Kwaadpepper commented Nov 23, 2022

I have got to the time to look at all of this yet, but I could make this suggestions, there are more benefits to store amounts in a smaller unit.
Like the price to be stored in cents.

I have worked with some payment apis and the do work with cents instead of euro.

This simplify calculations. Then you round strategy could be used for getters and setters

This is a big change but this PR is a big refactor already.

EDIT : My bad just saw the vendor Money package is for this usage looking at this branch code.

@bumbummen99
Copy link
Owner Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants