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

WIP: Fix float conversion #258

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

WIP: Fix float conversion #258

wants to merge 1 commit into from

Conversation

cuducos
Copy link
Contributor

@cuducos cuducos commented Oct 15, 2017

As described in #198:

  • the attempt detect the type of '1.0' (a string) would (correctly) suggest FloatField
  • but the attempt do detect the type of 1.0 (a float) would (wrongly) suggest IntegerField

This was probably due to this comparison in the IntegerField.deserialize method:

elif isinstance(value, float):
            new_value = int(value)
            if new_value != value:
        …

As indeed 1 == 1.0, this might be causing the wrongly type detection. Thus I'm suggesting is instead — in other words 1 == 1.0 passes, but 1 is 1.0 doesn't.

That fixes the issues and make tests suggested in the issue pass ; )

@cuducos
Copy link
Contributor Author

cuducos commented Nov 1, 2017

There's a problem in this PR. This PR brakes test_IntegerField (test_fields.py) in this assertion:

self.assertEqual(fields.IntegerField.deserialize(42.0), 42)

It makes sense to convert 42.0 as a float when using detect_type, but is user specifies IntegerField then IntegerField.deserialize(42.0) should return 42. Is there something in the API to know, at the point of deserializing, if the method was called from detect_type?

@cuducos cuducos changed the title Fix float conversion WIP: Fix float conversion Nov 1, 2017
@turicas turicas force-pushed the develop branch 2 times, most recently from fa144f0 to bbb2c57 Compare November 30, 2018 01:34
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.

1 participant