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

Fix --time-colon for negative time amounts #2064

Merged
merged 1 commit into from Sep 27, 2021

Conversation

rafasc
Copy link
Contributor

@rafasc rafasc commented Sep 25, 2021

While the current formula works for positive numbers, negative numbers
are incorrectly represented.

One of the issues comes from the fact that floor(x) < x for every x.
amount_t precision will always be a non negative number and the code
that attempts to fix the issue for negative number will never run.

If we truncate the number instead, the current formula works for both
positive and negative numbers without making negative numbers a corner
case. So let's do that.

While the current formula works for positive numbers, negative numbers
are incorrectly represented.

One of the issues comes from the fact that floor(x) < x for every x.
`amount_t precision` will always be a non negative number and the code
that attempts to fix the issue for negative number will never run.

If we truncate the number instead, the current formula works for both
positive and negative numbers without making negative numbers a corner
case. So let's do that.

Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com>
@rafasc
Copy link
Contributor Author

rafasc commented Sep 25, 2021

This should close #1176

Copy link
Contributor

@tbm tbm left a comment

Choose a reason for hiding this comment

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

I can't really evaluate this but since it makes the testcase work and since John is busy I'll approve this.

Thank you!

@tbm tbm merged commit ed58869 into ledger:master Sep 27, 2021
@tbm
Copy link
Contributor

tbm commented Sep 27, 2021

Thank you!

@tbm
Copy link
Contributor

tbm commented Nov 3, 2021

@CharlaLuetmer2001 does the fix from @rafasc work for you?

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.

None yet

2 participants