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

Modify Rounding methods #220

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Modify Rounding methods #220

merged 3 commits into from
Mar 14, 2021

Conversation

lovung
Copy link
Contributor

@lovung lovung commented Mar 7, 2021

Why

What I did

  • Change the name of the current methods
    • RoundUp -> RoundCeil
    • RoundDown -> RoundFloor
  • Add the new method with the names RoundUp and RoundDown.
  • Add unit test for the new methods

@lovung
Copy link
Contributor Author

lovung commented Mar 10, 2021

@mwoss Please help me to review this PR when you have time. Thank you so much.

@mwoss
Copy link
Member

mwoss commented Mar 11, 2021

I apologize @lovung for the late response. I was aware that you have created a PR, but I'm not feeling good lately, so I mostly sleep after my usual 9-7 work :<. I will try to review it today!

Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

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

I had the same thoughts when I was implementing RoundUp/Down, but I decided to go with those two rounding methods and in v2 introduce a single, parametrized Round method that will implement all possible rounding methods.
Anyways, I think adding Floor and Ceil rounding will be valuable and we would eventually add them before reasling v2.
One thing, I would adjust the docs of RoundUp and RooundDown as now they should be something like:
RoundUp rounds the decimal away from zero. and RoundDown rounds the decimal towards zero.

@lovung
Copy link
Contributor Author

lovung commented Mar 12, 2021

@mwoss Thank you so much for your opinion. I have already updated the docs for RoundUp and RoundDown at b28c608

@lovung
Copy link
Contributor Author

lovung commented Mar 12, 2021

in v2 introduce a single, parametrized Round method that will implement all possible rounding methods.

This is a good idea. Do you want me to help to implement it?

@mwoss
Copy link
Member

mwoss commented Mar 12, 2021

I was talking about the v2 after fixing all the current issues, so probably after v1.4. About v2 itself I was thinking about the introduction of NaN, Inf values, removing panics, adding Contex, proper precision handling, etc. So there would be much work to do.
If you are willing to help with current issues, I would more than happy to see more of your contributions to the decimal library :DD

@lovung
Copy link
Contributor Author

lovung commented Mar 14, 2021

@mwoss I will help if I can. I see some current issues that need to have Context to solve but some are the feature of decimal (don't need to solve).

And one more thing, I want to know when will you merge this PR to the master branch?

@mwoss
Copy link
Member

mwoss commented Mar 14, 2021

Fantastic, your help is always welcome. :)
I will merge this PR in a second

@mwoss mwoss merged commit f77bb07 into shopspring:master Mar 14, 2021
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