Add localization support to the FloatField #4925

kgeorgy · 2017-02-27T12:09:13Z

Description

Full description of the issue can be found here: Issue #4584.

The goals of this pull request is add the localize and coerce_to_string parameters, which are available with the DecimalField, to the FloatField.

This pull request is a simplification of the pull request #4586 as recommended by the maintainer.

refs #4584

Merge master from official django-rest-framework

…FloatField like it is done for the DecimalField, in order to use the user current local to serialise and deserialise the float value if required.

tomchristie · 2017-07-13T11:22:19Z

rest_framework/fields.py

@@ -939,7 +945,12 @@ def __init__(self, **kwargs):

    def to_internal_value(self, data):

-        if isinstance(data, six.text_type) and len(data) > self.MAX_STRING_LENGTH:
+        data = smart_text(data).strip()


Not keen on this. For the default behavior we'd now be unnecessarily transforming the incoming value into a string, and then back to a float again. Perhaps we could push this inside the if self.localize: block?

No because we could have a string as input without having the localize attribute to True.

I'll propose a change doing all string related sanitization only if the input is a string.

tomchristie · 2017-07-13T11:24:42Z

Looking good. One comment to address.
We also need documentation. That could go into this pull request (add to the serializer fields section). Alternatively I'd be happy with the documentation just added against this thread, so that I can then easily go ahead and update the documentation accordingly.

tomchristie · 2017-07-13T11:25:15Z

And apologies for the delay! This one seems to have slipped through the cracks for a bit.

* Do string sanitization only if input is a string

…rameters

kgeorgy · 2017-08-16T08:44:55Z

I had a bit of time to rework on this :) Then now the string related stuff are done only if the input of to_internal_value is a string (using six to ensure python 2 and 3 compatibility). This will ensure to not convert a float value to string and then back to float if the input is already a float.

coerce_to_string and localize parameters ares now documented.

Thank you for the review, this stuff starts to look good.

carltongibson · 2017-09-28T09:41:36Z

@kgeorgy I've milestoned this for 3.7.0. I'll review it, and we'll either have it or not for then.

Any chance you could rebase?

Thanks for the input!

kgeorgy · 2017-09-28T10:39:43Z

@carltongibson Done!

carltongibson · 2017-10-04T09:51:25Z

docs/api-guide/fields.md

 - `max_value` Validate that the number provided is no greater than this value.
 - `min_value` Validate that the number provided is no less than this value.
+- `localize` Set to `True` to enable localization of input and output based on the current locale. This will also force `coerce_to_string` to `True`. Defaults to `False`. Note that data formatting is enabled if you have set `USE_L10N=True` in your settings file.


Defaults to False

Do you mean None? It's a boolean field right? Should the default be False, rather than None?

Because it is the way it is in done in the DecimalField, and this pull request is just to apply the exact same principle to FloatField, but it is OK for me to use False as default.

carltongibson · 2017-10-04T09:52:25Z

rest_framework/fields.py

@@ -954,6 +956,10 @@ class FloatField(Field):
    def __init__(self, **kwargs):
        self.max_value = kwargs.pop('max_value', None)
        self.min_value = kwargs.pop('min_value', None)
+        self.localize = kwargs.pop('localize', False)
+        self.coerce_to_string = kwargs.pop('coerce_to_string', None)


Why not False here?

Same reason as in the previous point. But we can change both.

Can you just make this False (and in the docs signature too) — I think it's cleaner. Thanks!

carltongibson · 2017-10-04T09:54:12Z

rest_framework/fields.py

+        self.localize = kwargs.pop('localize', False)
+        self.coerce_to_string = kwargs.pop('coerce_to_string', None)
+        if self.localize:
+            self.coerce_to_string = True


Why do we need this here?

In to_representation:

if self.localize: return number_format(value) if self.coerce_to_string: ...

So if self.localize, self.coerce_to_string is never tested.

Yes, because if localize is True, we always coerce to string (number_format returns a string). We only needs to check coerce_to_string if localize is False. (And we override coerce_to_string to True in the init, so when localize is True, coerce_to_string must be True).

So we can drop this line...? (We don't need to override coerce_to_string since it will never be used.)

It could be used if we don't set localize to True, we could have localize = False and coerce_to_string = True.

Sure... Line 960 is fine. This comes into play in to_representation when localize is `False.

But line 961 and 962 are no-ops really (right?) — So (I think) we can delete them. Yes/no?

Ok got it... Yes it seems to... just come from a copy & past from line 989 and 990 in DecimalField.init. I'll remove it.

carltongibson

@kgeorgy I just have a couple of questions. (See comments) but looks good!

… be False insteed of None

kgeorgy · 2017-10-04T11:10:05Z

@carltongibson coerce_to_string is now defaulted to False (as it was documented), and the unneeded overriding lines are now removed.

carltongibson

OK. Awesome work. Thanks!

kgeorgy and others added 3 commits February 21, 2017 15:27

Merge pull request #1 from tomchristie/master

0316360

Merge master from official django-rest-framework

Merge commit 'b936d829a6b77fe344d2021ae41e89dd66eba4d4'

ddfce42

Add the "localize" and "coerce_to_string" constructor options to the …

38b3064

…FloatField like it is done for the DecimalField, in order to use the user current local to serialise and deserialise the float value if required.

jpadilla added the Enhancement label Mar 2, 2017

kgeorgy added 15 commits March 3, 2017 10:13

Merge branch 'master' into localizedfloatfield

878a0a5

Merge branch 'master' into localizedfloatfield

279ffc9

Merge branch 'master' into localizedfloatfield

7daa688

Merge branch 'master' into localizedfloatfield

095cd3a

Merge branch 'master' into localizedfloatfield

26c44b0

Merge branch 'master' into localizedfloatfield

479f323

Merge branch 'master' into localizedfloatfield

5fa2b4e

Merge branch 'master' into localizedfloatfield

8b5e479

Merge branch 'master' into localizedfloatfield

3772798

Merge branch 'master' into localizedfloatfield

9705179

Merge branch 'master' into localizedfloatfield

855dfcd

Merge branch 'master' into localizedfloatfield

7f543c8

Merge branch 'master' into localizedfloatfield

d94b00c

Merge branch 'master' into localizedfloatfield

c737fa0

Merge branch 'master' into localizedfloatfield

5094299

tomchristie mentioned this pull request Jul 13, 2017

Add localization support for all number fields (Issue #4584) #4586

Closed

tomchristie added this to the 3.6.4 Release milestone Jul 13, 2017

tomchristie reviewed Jul 13, 2017

View reviewed changes

kgeorgy and others added 4 commits August 16, 2017 08:35

Merge branch 'master' into localizedfloatfield

f389833

* Restore imports in order to make isort test pass

bf53f74

* Do string sanitization only if input is a string

Fix string identification in python2.7

fe0c4f9

Add documentation for the coerce_to_string and localize FloatField pa…

a0c8d2f

…rameters

carltongibson added this to the 3.6.5 Release milestone Aug 21, 2017

carltongibson removed this from the 3.6.4 Release milestone Aug 21, 2017

carltongibson modified the milestones: 3.6.5 Release, 3.7.0 Release Sep 28, 2017

kgeorgy changed the base branch from master to version-3.7 September 28, 2017 09:50

Merge branch 'version-3.7' into localizedfloatfield

9bd6229

carltongibson reviewed Oct 4, 2017

View reviewed changes

Removes unneeded lines, and set default value for coerce_to_string to…

8890539

… be False insteed of None

Update FloatField signature docs

5dd01c6

carltongibson approved these changes Oct 4, 2017

View reviewed changes

carltongibson merged commit 52da527 into encode:version-3.7 Oct 4, 2017

carltongibson mentioned this pull request Oct 4, 2017

FloatField should use the user current local to parse float value #4584

Closed

kgeorgy deleted the localizedfloatfield branch October 4, 2017 13:05

matthewhegarty mentioned this pull request Oct 21, 2022

Float and Decimal widgets use LANGUAGE_CODE on export django-import-export/django-import-export#1501

Merged

Uh oh!

Add localization support to the FloatField #4925

Add localization support to the FloatField #4925

Uh oh!

Conversation

kgeorgy commented Feb 27, 2017

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomchristie commented Jul 13, 2017

Uh oh!

tomchristie commented Jul 13, 2017

Uh oh!

kgeorgy commented Aug 16, 2017

Uh oh!

carltongibson commented Sep 28, 2017

Uh oh!

kgeorgy commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgeorgy Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

kgeorgy commented Oct 4, 2017

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kgeorgy Oct 4, 2017 •

edited

Loading