Re: Why is from_db_value not called on ModelClass.objects.create()?

2015-09-28 Thread Gordon
Thanks for the replies and suggestions Simon and Marten.

I am definitely calling form.clean() because I am using the default admin 
for views in my tests.  The field in the case that was tripping me up is 
being used as an auto-populating primary key... so the admin form was 
excluding it from clean() and then redirecting to the wrong url.

Since the behavior is written and working as intended I can tackle this 
particular problem from a different direction.  But in my mind, it still 
seems like ModelClass.objects.create() should return an instance as if it 
were pulled from the database... especially with convenience methods like 
`get_or_create` because the field value would be different in this scenario 
depending on if the method uses get or create to return the instance (I 
haven't looked at the implementation but the name would imply it relies on 
the create method).  But I guess there are cases where you don't need to 
use the returned instance and would waste a few cpu cycles.  This issue 
makes `get_or_create` a little dangerous I think.

I think it adheres to the principle of least surprise since it makes Django
> models behave just like normal Python classes in this regard.
>

I am not sure about this since ModelClass.objects.create() is a manager 
method that returns a new model instance and not the manager "self".  But 
after rereading the documentation about the `create` method it does provide 
an alternate equivalent syntax that I would expect this behavior from.


-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/c7d694d5-74f3-41d0-b21d-a860fc7b4c28%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Re: Why is from_db_value not called on ModelClass.objects.create()?

2015-09-28 Thread Marten Kenbeek
I've had to explain this behaviour a few times - most recently this SO 
question 

 - 
so 
I think it's worth it to explicitly document this behaviour. I agree with 
Simon, though - I don't 
think the behaviour itself should change. 

You can always call refresh_from_db() 

 
to reload the values from the database. This method
was added in 1.8. Just a quick idea: adding a flag to Model.save() to call 
refresh_from_db() 
after the model is saved might make it more accessible, and it makes it 
clearer that the values 
on the model aren't changed if you don't explicitly reload the model. 

Marten

On Monday, September 28, 2015 at 10:36:09 PM UTC+2, charettes wrote:
>
> Hi Gordon,
>
> That's what I would expect the cash attribute to be since it was not 
> retrieved
> from the database but explicit set during initialization. To me the 
> `from_db_value`
> method name makes it clear it is only called in the former case. If you 
> want
> the object you created to have a certain value pass them as desired:
>
> CashModel.objects.create(cash=Cash('12.50'))
>
> or call clean() on your model instance to make sure CashField.to_python() 
> is called.
>
> Note that this behavior can also be exhibited by assigning a value to an 
> already
> existing instance.
>
> instance = CashModel.objects.get()
> instance.cash = '12.50'
> assert instance.cash == '12.50'
>
> I think it adheres to the principle of least surprise since it makes Django
> models behave just like normal Python classes in this regard.
>
> e.g.
>
> class Foo:
> def __init__(self, bar):
> self.bar = bar
>
> assert Foo('value').bar == 'value'
>
> If you really want to make sure assignment for a specific field
> sets an altered value on an instance the you'll have attach a
> descriptor on the model class and make sure to call
> CashField.to_python on __set__(). Django does this for
>
> Simon
>
> Le lundi 28 septembre 2015 15:10:54 UTC-4, Gordon a écrit :
>>
>> I originally asked this on IRC.  It was suggested to raise the question 
>> in the mailing list. I've copied the log below with irrelevant lines 
>> removed (dpastes provided after the quoted irc)
>>
>>  Why is from_db_value not called on Cls.objects.create()?  For 
>>> example, in the CashModel test, why isn't it tested that cash is an 
>>> instance of Cash after create? 
>>
>>  
>>> https://github.com/django/django/blob/master/tests/from_db_value/tests.py
>>
>>  Something like http://dpaste.com/1R69S5F  I seem to be getting the 
>>> wrong value for a custom primary key in the admin because of this
>>
>>  *only on the create redirect
>>
>>  Or is this expected to be handled by the field?
>>
>>  gp: this seems similar to #24028
>>
>>  timograhm: would that test 'test_create()' in the dpaste fit in with 
>>> the from_db_value?  Or should the create test be somewhere else?
>>
>>  seems okay there, does it pass or are you proposing a 
>>> behavior change?
>>
>>  When I was trying to figure out why my values were incorrect I tried 
>>> that test and it failed.  But I would need to verify in a clean project 
>>> before sending a pr
>>
>>  but I think the actual fix is outside of my knowledge
>>
>>  This ugly hack seems to fix it on my field if that means anything to 
>>> you http://dpaste.com/3J0C5DD
>>
>>  gp: it seems like it could be a useful behavior. I guess it 
>>> has probably been discussed before but I'm not aware of the discussion. 
>>> Maybe it would be worth raising on the mailing list and at least 
>>> documenting the reasons for the current behavior if it can't be changed due 
>>> to backwards compatibility.
>>
>>
>>  The first dpaste (1R69S5F) is a copy of the from_db_value/tests.py with 
>> the following modifications:
>>
>> class FromDBValueTest(TestCase):
>> def setUp(self):
>> self.obj = CashModel.objects.create(cash='12.50')
>>
>> def test_create(self):
>> self.assertIsInstance(self.obj.cash, Cash)
>>
>>
>> The second dpaste (3J0C5DD) is the following:
>>
>> def contribute_to_class(self, cls, name):
>> super(IntegerIdentifierBase, self).contribute_to_class(cls, name)
>> cls_save = cls.save
>> def save_wrapper(obj, *args, **kwargs):
>> cls_save(obj, *args, **kwargs)
>> value = getattr(obj, self.attname)
>> value = self.to_python(value)
>> setattr(obj, self.attname, value)
>> cls.save = save_wrapper
>>
>>
>>
>>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at 

Re: Why is from_db_value not called on ModelClass.objects.create()?

2015-09-28 Thread charettes
Hi Gordon,

That's what I would expect the cash attribute to be since it was not 
retrieved
from the database but explicit set during initialization. To me the 
`from_db_value`
method name makes it clear it is only called in the former case. If you want
the object you created to have a certain value pass them as desired:

CashModel.objects.create(cash=Cash('12.50'))

or call clean() on your model instance to make sure CashField.to_python() 
is called.

Note that this behavior can also be exhibited by assigning a value to an 
already
existing instance.

instance = CashModel.objects.get()
instance.cash = '12.50'
assert instance.cash == '12.50'

I think it adheres to the principle of least surprise since it makes Django
models behave just like normal Python classes in this regard.

e.g.

class Foo:
def __init__(self, bar):
self.bar = bar

assert Foo('value').bar == 'value'

If you really want to make sure assignment for a specific field
sets an altered value on an instance the you'll have attach a
descriptor on the model class and make sure to call
CashField.to_python on __set__(). Django does this for

Simon

Le lundi 28 septembre 2015 15:10:54 UTC-4, Gordon a écrit :
>
> I originally asked this on IRC.  It was suggested to raise the question in 
> the mailing list. I've copied the log below with irrelevant lines 
> removed (dpastes provided after the quoted irc)
>
>  Why is from_db_value not called on Cls.objects.create()?  For 
>> example, in the CashModel test, why isn't it tested that cash is an 
>> instance of Cash after create? 
>
>  
>> https://github.com/django/django/blob/master/tests/from_db_value/tests.py
>
>  Something like http://dpaste.com/1R69S5F  I seem to be getting the 
>> wrong value for a custom primary key in the admin because of this
>
>  *only on the create redirect
>
>  Or is this expected to be handled by the field?
>
>  gp: this seems similar to #24028
>
>  timograhm: would that test 'test_create()' in the dpaste fit in with 
>> the from_db_value?  Or should the create test be somewhere else?
>
>  seems okay there, does it pass or are you proposing a 
>> behavior change?
>
>  When I was trying to figure out why my values were incorrect I tried 
>> that test and it failed.  But I would need to verify in a clean project 
>> before sending a pr
>
>  but I think the actual fix is outside of my knowledge
>
>  This ugly hack seems to fix it on my field if that means anything to 
>> you http://dpaste.com/3J0C5DD
>
>  gp: it seems like it could be a useful behavior. I guess it 
>> has probably been discussed before but I'm not aware of the discussion. 
>> Maybe it would be worth raising on the mailing list and at least 
>> documenting the reasons for the current behavior if it can't be changed due 
>> to backwards compatibility.
>
>
>  The first dpaste (1R69S5F) is a copy of the from_db_value/tests.py with 
> the following modifications:
>
> class FromDBValueTest(TestCase):
> def setUp(self):
> self.obj = CashModel.objects.create(cash='12.50')
>
> def test_create(self):
> self.assertIsInstance(self.obj.cash, Cash)
>
>
> The second dpaste (3J0C5DD) is the following:
>
> def contribute_to_class(self, cls, name):
> super(IntegerIdentifierBase, self).contribute_to_class(cls, name)
> cls_save = cls.save
> def save_wrapper(obj, *args, **kwargs):
> cls_save(obj, *args, **kwargs)
> value = getattr(obj, self.attname)
> value = self.to_python(value)
> setattr(obj, self.attname, value)
> cls.save = save_wrapper
>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/fd5fab91-8574-4c61-9c12-ddb6c38d486b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Why is from_db_value not called on ModelClass.objects.create()?

2015-09-28 Thread Gordon
I originally asked this on IRC.  It was suggested to raise the question in 
the mailing list. I've copied the log below with irrelevant lines 
removed (dpastes provided after the quoted irc)

 Why is from_db_value not called on Cls.objects.create()?  For example, 
> in the CashModel test, why isn't it tested that cash is an instance of Cash 
> after create? 

 
> https://github.com/django/django/blob/master/tests/from_db_value/tests.py

 Something like http://dpaste.com/1R69S5F  I seem to be getting the 
> wrong value for a custom primary key in the admin because of this

 *only on the create redirect

 Or is this expected to be handled by the field?

 gp: this seems similar to #24028

 timograhm: would that test 'test_create()' in the dpaste fit in with 
> the from_db_value?  Or should the create test be somewhere else?

 seems okay there, does it pass or are you proposing a behavior 
> change?

 When I was trying to figure out why my values were incorrect I tried 
> that test and it failed.  But I would need to verify in a clean project 
> before sending a pr

 but I think the actual fix is outside of my knowledge

 This ugly hack seems to fix it on my field if that means anything to 
> you http://dpaste.com/3J0C5DD

 gp: it seems like it could be a useful behavior. I guess it 
> has probably been discussed before but I'm not aware of the discussion. 
> Maybe it would be worth raising on the mailing list and at least 
> documenting the reasons for the current behavior if it can't be changed due 
> to backwards compatibility.


 The first dpaste (1R69S5F) is a copy of the from_db_value/tests.py with 
the following modifications:

class FromDBValueTest(TestCase):
def setUp(self):
self.obj = CashModel.objects.create(cash='12.50')

def test_create(self):
self.assertIsInstance(self.obj.cash, Cash)


The second dpaste (3J0C5DD) is the following:

def contribute_to_class(self, cls, name):
super(IntegerIdentifierBase, self).contribute_to_class(cls, name)
cls_save = cls.save
def save_wrapper(obj, *args, **kwargs):
cls_save(obj, *args, **kwargs)
value = getattr(obj, self.attname)
value = self.to_python(value)
setattr(obj, self.attname, value)
cls.save = save_wrapper



-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/3df06313-41a4-42ab-8fc6-f1cdeee7b515%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.