djangonauts / django-hstore

PostgreSQL HStore support for Django.
http://django-hstore.readthedocs.io/
Other
517 stars 142 forks source link

implement schema and types as discussed in #48 #54

Closed nemesifier closed 10 years ago

nemesifier commented 10 years ago

The attempt made in #48 yelded interesting results but the code got very messy and diverged a bit from master (which went ahead in the meanwhile).

This is a new attempt, I hope to write cleaner code and get closer to something usable.

For the entire discussion see #48.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.12%) when pulling 5441a70f49c95d70e2c0bdac2fe744df5f3cc70c on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.1%) when pulling f472d3023c9a2274e854bb126b852ee848aeb868 on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

nemesifier commented 10 years ago

@niwibe I managed to get most of the stuff working, except the admin which for some kind of reason is not persisting the changes into the database.

any hint?

niwinz commented 10 years ago

I have viewed the code, and I have some doubt: why the virtual field on tests inherits from integerfield. Making this inheritance django creates a real database field for this model field in any case... and it seems not the correct behavior for virtual fields.

In my opinion, virtual fields should be clear python classes with descriptor protocol (as your mixin). That, make it work with admin, should be very simple, make the admin form and add the appropriate form field. The unique drawback of not having this inheritance is that admin model form not auto-discover this field.

In any case I don't like have useless database fields for virtual fields.

nemesifier commented 10 years ago

Hey @niwibe, i have to correct you! EHEH

First of all, just to be sure, i checked right now the DB and there is no additional column in the database, even after running syncdb several times. And I'll explain you why.

Let's see the virtual.py file: https://github.com/djangonauts/django-hstore/blob/f472d3023c9a2274e854bb126b852ee848aeb868/django_hstore/virtual.py

Do you see that it is a mixin object? It is designed to be mixed in with standard django fields to make them virtual. I'm trying this way so that we can manage to have all those awsome fields that django already has used by our extension. Now let's see what is hardcoded (temporarily) in the test model: https://github.com/djangonauts/django-hstore/blob/f472d3023c9a2274e854bb126b852ee848aeb868/tests/django_hstore_tests/models.py#L88 When you create a class, if you extend from 2 base classes, the one on the left overwrites the methods of the one on the right. So the methods defined in HStoreVirtualMixin will overwrite the base field from which we are extending.

Check the contribute_to_class method, what it does is to pass "virtual_only" True to its parent contribute_to_class method, which makes it indeed virtual (no real DB column). (this method could be also entirely overwritten like the GenericForeignKey in contrib does).

this line: https://github.com/djangonauts/django-hstore/blob/f472d3023c9a2274e854bb126b852ee848aeb868/django_hstore/virtual.py#L16 effectively registers itself as the descriptor, and if you check you will see that the mixin has the main descriptor methods: get https://github.com/djangonauts/django-hstore/blob/f472d3023c9a2274e854bb126b852ee848aeb868/django_hstore/virtual.py#L20 set https://github.com/djangonauts/django-hstore/blob/f472d3023c9a2274e854bb126b852ee848aeb868/django_hstore/virtual.py#L29

I've been studying and emulating how other similar fields are done, for example the GenericForeignKey in django.contrib.contenttypes.

My idea is that once I get this mechanism working I can create those VirtualField(s) on the fly with meta-programming (instead of hardcoding) by parsing the schema attribute.

So, until now, I have been able to do most of the basic operations that I need before proceding into the next step, which are:

But i have not been able to frigging understand why saving the value of the number doesn't work in the admin, nor in the add form nor the change form.

I think i'll delve deeper into the generic field to understand why.

In the meanwhile, you can tell me if I clarified your doubt and if you have any other doubts. Why don't you try to test this branch on your own and hack it a bit?

nemesifier commented 10 years ago

PS: i'm testing on python 2.7 and django 1.6.5

nemesifier commented 10 years ago

woa I made it!

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.1%) when pulling e96b7c4820ad5b36881fe3c7971261f17223d3ce on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

nemesifier commented 10 years ago

@niwibe i suspect it is not possible to support django 1.5 for this feature, I tried everything yesterday to no avail. Django 1.6 and 1.7 are ok though. Now I just need to fix python3 and put some conditional statements that do not execute the SchemaTests for django < 1.6.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.42%) when pulling 431e219abc253ce311824df02e3b7bf589334698 on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

niwinz commented 10 years ago

Sorry for lack of help, this days I'm completely out.

I have reviewed it and seems very well done. I put some comment but is not important.

Thanks for you work! ;)

nemesifier commented 10 years ago

@niwibe :+1: send more comments if you like, i'll be happy to improve it, the latest commit should fix the remaining failing build for python2.6

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.43%) when pulling 8a346a9ae58c6320cb21dc56fbc22c190a877a93 on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.3%) when pulling 8229e850e1631d8fd038436d2aa1fccb26b9a699 on features/schema into 5adad3fc606b696d8915a7ed28b8b79728250687 on master.

nemesifier commented 10 years ago

I'm merging hoping that someone else apart me will try the development version and send feedback.