search

mirumee / django-prices

Django fields for the prices module
158 stars 53 forks source link

TaxedMoneyField should be Django Field type #61

Closed akjanik closed 6 years ago

akjanik commented 6 years ago

Right now there is:

class TaxedMoneyField(object):
...

is being used and named as a Field, so I guess it should also be type of some Django Field, not object.

rafalp commented 6 years ago

Is there any error or issue caused by this field not extending models.Field?

akjanik commented 6 years ago

Yes, I encountered this problem while trying to implement GraphQL endpoints for Saleor's Order which uses TaxedMoneyField for two fields. https://github.com/graphql-python/graphene-django/blob/master/graphene_django/utils.py#L46 this line throws an error:

  File "/Users/adam/projects/saleor/.direnv/python-3.6.4/lib/python3.6/site-packages/graphene_django/utils.py", line 47, in get_model_fields
    list(model._meta.local_many_to_many))
TypeError: '<' not supported between instances of 'TaxedMoneyField' and 'CharField'

as sorted does not know how to compare Django's Field and object.

I quick fixed it in my local lib by changing object to DecimalField (not sure if this one would correct) and problem was solved, but had to also define help_text and null attributes, not sure if this is related to the issue or not.

patrys commented 6 years ago

DecimalField surely would not be correct as it represents a database type and TaxedMoneyField does not. As it's quite similar to the GenericForeignKey maybe it could use the same base class or one inspired by it?