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

handle Decimal on amount values #364

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

handle Decimal on amount values #364

wants to merge 3 commits into from

Conversation

laf-rge
Copy link
Contributor

@laf-rge laf-rge commented Aug 9, 2024

In the testing on my project the jsonEncoder in newer versions of python calls default for both keys and values. This means that you get an exception if you pass in Decimal as a line amount on newer versions of python with the existing lambda on default.

I think we should also include a test where a bill is created with a decimal line amount and to_json() is called to ensure there is no exception.

@bradywhitby
Copy link

I'd just like to express a desire to have this PR merged into master. Our program works fine on 0.9.8, but fails in 0.9.9+ due to this jsonEncoder issue

@laf-rge
Copy link
Contributor Author

laf-rge commented Oct 7, 2024

Let me know if there is anything else I can do to get this merged.

@laf-rge
Copy link
Contributor Author

laf-rge commented Dec 29, 2024

Pinging this again would love to get this out.

@ljwobker
Copy link

ljwobker commented Jan 4, 2025

I'm also running into this on a formerly working deployment - not sure exactly where it broke for me or which set of conditions are the trigger, but I'm pretty confident that the issue is the same :(

@laf-rge
Copy link
Contributor Author

laf-rge commented Jan 4, 2025

Could you test with this branch and see if it fixes the issue?

@ljwobker
Copy link

ljwobker commented Jan 5, 2025

Well... your single user test passed for me! ;-)

@Uniquely6958
Copy link

Hi
I caught this issue myself this week. I found this PR when trying to create one myself, so I thought I'd add my thoughts here instead.
I notice that you do not need to have the DecimalEncoder at all if you have added the condition for checking Decimals in the to_json function. It is redundant and does not provide any assistance, The same is achieved with

class ToJsonMixin(object):
    def to_json(self):
        return json.dumps(self, default=self.json_filter(), sort_keys=True, indent=4)

    def json_filter(self):
        """
        filter out properties that have names starting with _
        or properties that have a value of None
        """
        return lambda obj: str(obj) if isinstance(obj, decimal.Decimal) else dict((k, v) for k, v in obj.__dict__.items()
                                if not k.startswith('_') and getattr(obj, k) is not None)

@laf-rge
Copy link
Contributor Author

laf-rge commented Jan 12, 2025

I added a unit test that can help you play with your fix. I didn't get it to work well in the bill example with nested objects.

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.

4 participants