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

Always pass a valid rounding value #6107

Closed
wants to merge 3 commits into from

Conversation

kbussell
Copy link

@kbussell kbussell commented Aug 4, 2018

Description

Fixes #6105

Prevent an exception in quantize() when monkey-patching the decimal
library as cdecimal in Python 2 environments by always passing a valid
(not None) rounding value to quantize().

Fixes encode#6015

Prevent an exception in `quantize()` when monkey-patching the decimal
library as cdecimal in Python 2 environments by always passing a valid
(not None) `rounding` value to `quantize()`.
@codecov-io
Copy link

codecov-io commented Aug 4, 2018

Codecov Report

Merging #6107 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6107      +/-   ##
==========================================
+ Coverage   96.18%   96.18%   +<.01%     
==========================================
  Files         128      128              
  Lines       17624    17636      +12     
  Branches     1459     1459              
==========================================
+ Hits        16951    16963      +12     
  Misses        465      465              
  Partials      208      208

@rpkilby
Copy link
Member

rpkilby commented Aug 4, 2018

I'm tempted to close this per #5741 (comment), but my off the cuff reaction is that this seems like a clean way to support Python 2.

Copy link
Member

@rpkilby rpkilby left a comment

Choose a reason for hiding this comment

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

Hi @kbussell, a minimal test that acts as a sanity check for Python 2 compat would be appreciated.

@@ -1135,11 +1135,12 @@ def quantize(self, value):
return value

context = decimal.getcontext().copy()
rounding = context.rounding if self.rounding is None else self.rounding
Copy link
Member

Choose a reason for hiding this comment

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

I would add a note that this exists for Python 2 compatibility.

@kbussell kbussell force-pushed the 6105-cdecimal-rounding branch 3 times, most recently from 3ef0da3 to f09cd0c Compare August 7, 2018 22:32
@kbussell kbussell force-pushed the 6105-cdecimal-rounding branch from f09cd0c to 602eec8 Compare August 7, 2018 22:55
@kbussell
Copy link
Author

kbussell commented Aug 7, 2018

Hi @rpkilby, I've added a comment and a test that exercises the code in Python 2.7 using cdecimal

@rpkilby rpkilby added this to the 3.9 Release milestone Oct 16, 2018
@rpkilby
Copy link
Member

rpkilby commented Oct 16, 2018

cc @tomchristie. Do you want to review this for the 3.9 release, since this will likely be the last release that supports Python 2.7 (#6230)?

@tomchristie
Copy link
Member

Thanks for taking the time to PR this!

I think I'd like us to pass, particularly given how close we are to phasing out Python 2 here. I'd suggest explicitly using a DecimalField subclass on your serializers in order to support this use-case.

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