Skip to content

Conversation

tehfink
Copy link

@tehfink tehfink commented Jan 6, 2018

Django-2.0 compatibility

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 75.816% when pulling b2f9be7 on tehfink:Django-2.0 into 3191b61 on timsavage:master.

@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage remained the same at 75.816% when pulling 42d172f on tehfink:Django-2.0 into 3191b61 on timsavage:master.

Copy link
Owner

@timsavage timsavage left a comment

Choose a reason for hiding this comment

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

Can the Travis configuration be updated to include the targetted Django version?

I will likely drop support for some of the older versions of Django, however, any LTS version must be maintained.

owner = models.ForeignKey(USER_MODEL_NAME, verbose_name=_('owner'),
related_name='%(app_label)s_%(class)s_owner')
related_name='%(app_label)s_%(class)s_owner',
on_delete=models.CASCADE, null=True, blank=False)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason for null=True?

Copy link
Author

Choose a reason for hiding this comment

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

It's the only way 'makemigrations' would work for me; but perhaps there is a better way?

if value is not None:
value = value._amount
return connection.ops.value_to_db_decimal(value, self.max_digits, self.decimal_places)
return connection.ops.adapt_decimalfield_value(value, self.max_digits, self.decimal_places)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this change backwards compatible with Django < 2?

This might require a conditional to handle previous Django releases

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, I don't know, as everything I'm doing now is Django 2+. But it seems that 'value_to_db_decimal' disappeared in Django 1.9

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage remained the same at 75.816% when pulling 926aa18 on tehfink:Django-2.0 into 192650e on timsavage:master.



class Money(object):
class Money(decimal.Decimal):
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the right solution to this problem. This class composes a money value from a Decimal amount and a currency it does not use inheritance.

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.

3 participants