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

use field.value_to_string(model) for any non protected type #40

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

thenewguy
Copy link

Fix issue #39

@gasman
Copy link
Contributor

gasman commented Sep 29, 2015

Thanks! Could you add a unit test for this, please?

@thenewguy
Copy link
Author

@gasman: Yes. I made a comment about tests in the issue. It would have been more appropriate here

@thenewguy
Copy link
Author

@gasman I am having trouble running tests against wagtail to make sure I have a failing test before and successful test after changeset. I posted on wagtail-dev at https://groups.google.com/forum/#!topic/wagtail-developers/Q-JeY81IOLU

@thenewguy
Copy link
Author

So I've added a test. I've pushed the test to a separate branch as well without the fix so you can easily compare pass/fail.

The issue I am observing appears to be split between wagtail and modelcluster. The modelcluster case occurs with a related field pointing to a field that returns a custom object.

@thenewguy
Copy link
Author

If my previous comments emailed to you, ignore them. I attached SubfieldBase to the model instead of the ModelField in a copy/paste error from having them all in the same file.

But the models for the issue test case really needs the from_db_value() methods introduced in Django 1.8 without some ugly hacks. I think it would be better to keep the test model/field simple and skip the test on Django 1.7.

@thenewguy
Copy link
Author

So I've hit this in several places under wagtail too while trying to use a custom field that returns an object that cannot be natively serialized to json. Due to the nature of this particular field it is going to be faster for me to change the value's base classes to be compatible with json.dumps. It would probably be a good idea to add a model like this to your serialization tests since Django added the from_db_value method in 1.8.

These are related:
wagtail/wagtail#1752
wagtail/wagtail#1753

@gasman
Copy link
Contributor

gasman commented Oct 9, 2015

Hi @thenewguy - afraid I'm not really keen on all the Foos and Bars in the test case, as they obscure the details of what's being tested. Would it be possible to update these to a more "real-world" example? It looks like the CashField from the Django tests https://github.com/django/django/blob/master/tests/from_db_value/models.py would be a good candidate. Happy to have a go at this myself, but it might be a week or two before I get round to it...

Having skimmed over the Django docs to refamiliarise myself with from_db_value and the pre-1.8 equivalent SubfieldBase, I'm happy to keep this as a 1.8-specific test. It may be that old-style SubfieldBase custom fields are buggy too (and may or may not be fixed by the fix here), but lack of a fix/test for that shouldn't block us from fixing the behaviour for new-style from_db_value fields.

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.

2 participants