-
Notifications
You must be signed in to change notification settings - Fork 6
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
Improved up/downgrading breaks LedgerSMB::PGNumber #11
Comments
Solved this as an issue on our side. Sorry for the noise. |
The methods in If downgrading is enabled, things work differently. If I have not been able to install and run |
Ah, I didn't see that you have solved this issue already, so never mind my previous comment. But I am glad you were able to solve it. |
I have no idea how this happened, but up- and downgrading were enabled, outside of what I think happens in our codebase. No idea where the downgrades are coming from, but: setting downgrading explicitly to |
I noticed the latest commit in the LedgerSMB repo, and I am glad this solves the issue, but I really wonder why you experienced the issue. I see nothing in your codebase that enables up- and downgrading, nor do I see anything in |
I'm afraid that that commit wasn't enough; our 1.11.0 Docker image has the problem. I'll be looking to see if I can have a sequence of steps to reproduce the problem in our application. It manifests itself in multiple places: Numbers are assumed to be PGNumber-s, which means they're expected to have a I'll try to produce a complete reproduction recipe on our code base; after that I could probably use some help trying to reduce the reproduction to something small enough for you to work on... |
I found the culprit: |
Well, that is: the fact that Math::BigFloat/Math::BigInt now has improved up/downgrading in combination with the fact that Locale::CLDR uses 'bignum' and the fact that LedgerSMB uses its own numeric class |
Ok. So, to work around this, I wanted to load |
I am glad you found culprit. However, I believe the real problem is the fundamentally flawed object oriented design of the I have thought a lot about the issue you are experiencing and have a few suggestions:
|
So do I. I don't think there's much you can do about that while remaining backward compatible. So, I'm thinking the fix on our side - which I can only implement on the next round of big releases (because we just released a big one), is to create my own class which wraps Math::BigFloat instead of inheriting from it. That way, BigFloat and bignum can do what they want/need, but my code won't see the direct effects of it, since the wrapped value is expected to be a Math::* type. |
I'm going to look at your suggestions, but have a terrible busy week this week. I'll let you know what comes out of them as soon as I've tried. |
I had a look at this one (by way of the author of Locale::CLDR releasing a trial version) and it works like a charm! |
@ThePilgrim released a new version of Locale::CLDR (v0.34.2) which doesn't use 'bignum' and works like a charm (verified by the original reporter on our software). |
In LedgerSMB, we have a class called
LedgerSMB::PGNumber
which ultimatelyISA
Math::BigFloat
. Since this class adds a few methods, it's important that the result of each operation remains of the same class. While this used to work pretty well (mostly deployed on Debian versions), this is now broken (on Debian Bookworm). Going back to Bullseye and installing the latest Math::BigInt distribution from CPAN breaks the same way.Reading CHANGES, I suspect
1.999830
and1.999831
: Bullseye has1.999818
and Bookworm has1.999838
.Could you please indicate whether the current state is permanent? If so, what is your advice as to how to deal with the situation?
Should you need to look at the code, the
LedgerSMB::PGNumber
code is here. It's inheriting from Math::BigFloat through hereThe text was updated successfully, but these errors were encountered: