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

Improve DecimalField for easier subclassing. #2695

Merged
merged 1 commit into from Jun 4, 2015

Conversation

delinhabit
Copy link
Contributor

@delinhabit delinhabit commented Mar 14, 2015

Hello,

I did some cleanup in the DecimalField class to make the methods shorter and more focused on their responsibility, but the main goal is to allow developer do to something like this:

class PermissiveDecimalField(serializers.DecimalField):
    """
    Consider the configured `max_digits` and `decimal_places` for both
    serialization and deserialization, but do not reject input values with 
    bigger precisions.
    """
    def validate_precision(self, value):
        return self.quantize(value)

Without this refactor, covering such a use-case is quite ugly. You have to copy paste the to_internal_value to benefit for the sanity checks and then override the precision validation part:

class PermissiveDecimalField(serializers.DecimalField):
    """
    Consider the configured `max_digits` and `decimal_places` for both
    serialization and deserialization, but do not reject input values with 
    bigger precisions.
    """
    def to_internal_value(self, data):
        data = smart_text(data).strip()
        if len(data) > self.MAX_STRING_LENGTH:
            self.fail('max_string_length')

        try:
            value = decimal.Decimal(data)
        except decimal.DecimalException:
            self.fail('invalid')

        # Check for NaN. It is the only value that isn't equal to itself,
        # so we can use this to identify NaN values.
        if value != value:
            self.fail('invalid')

        # Check for infinity and negative infinity.
        if value in (decimal.Decimal('Inf'), decimal.Decimal('-Inf')):
            self.fail('invalid')

        return self.validate_precision(value)

    def validate_precision(self, value):
        context = decimal.getcontext().copy()
        context.prec = self.max_digits
        return value.quantize(
            decimal.Decimal('.1') ** self.decimal_places,
            context=context)

And if you think about it, it's not an obscure use-case at all. In my case, we are storing longitude and latitude values of up to 6 decimal places (~100 millimeters precision, which is more than enough for us) but the values can come from different geolocation sources which can have bigger precision and without this change we would have to sanitize the inputs on the client side. On each different client that you have. This way, I don't care about the infinitesimal precisions, I just grab the value, quantize it to the desired precision and store it in the database.

I like this sort of refactoring because if makes the code cleaner and the life of the developers easier. (I'm happy to do more of these if the community agrees that it's valuable).

Any thoughts?

Thanks!

@delinhabit
Copy link
Contributor Author

delinhabit commented Mar 26, 2015

Are there any concerns about this PR?

tomchristie added a commit that referenced this pull request Jun 4, 2015
[enhancement] Refactored DecimalField to allow easier subclassing
@tomchristie tomchristie merged commit e8cc948 into encode:master Jun 4, 2015
1 check passed
@tomchristie tomchristie added this to the 3.1.3 Release milestone Jun 4, 2015
@tomchristie
Copy link
Member

tomchristie commented Jun 4, 2015

Apologies for the delay!

@xordoquy xordoquy changed the title [enhancement] Refactored DecimalField to allow easier subclassing Improve DecimalField for easier subclassing. Jun 4, 2015
@delinhabit delinhabit deleted the refactor-decimalfield branch Jun 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants