Being able to instantiate specializations that are not final

[sebpiq]

I have the following hierarchy of models :

Machine
| |
V V
Work Personal
|
V
CI

When creating any object, then you can't call "specializations" manager on its model anymore ... for example :
>>> ci = CI() ; ci.save()
>>> CI.specializations.all()
Traceback .....
KeyError: u'/work/ci/'

I see what's the problem, and I guess there is a design decision to be taken :

non-final specializations can't be instantiated and final specializations don't have a "specializations" manager
OR
everything can be instantiated, and then "CI._meta.specializations" needs to contain "/work/ci/" as well (i.e. each GeneralizationModel contains also itself as a possible specialization)
OR
"specializations" manager excludes the instances of the calling model class

I can try to implement any of those :)

[sebpiq]

Similarly when specialization is not final, no default value is given for the field "specialization_type" when instantiating : https://github.com/2degrees/djeneralize/blob/master/djeneralize/models.py#L195

[euangoddard]

Why would you want to take a specializaed model class and do CI.specializations.all()? You know that this class is specialized and so you can do CI.objects.all().

Alternatively, if you want to get all Machine specialization you can do Machine.specializations.all().

Unless you cannot know in advance which class you'll be using at the start of the call, I think the issue can be avoided by select calls to the objects in question. To me, CI has no specializations and so CI.specializations.all() is nonsense. However, as the name of your ticket suggests, the error message is not that helpful, but then again, it seems like a lot of work to provide a helpful error message for a use case which is not meant to arise.

If you'd like to fork the repo and add a helpful message for that case, I'd be happy to consider it! Thanks

[sebpiq]

Well, ... as I found out after naming the issue, the main problem is that when you create an instance of a GeneralizationModel, subsequent calls to "specializations" on that model will fail, whether such calls make sense or not (For example, instantiating Work, and then calling Work.specializations fails, because '/work/' is not a specialization of itself, but still appears in the queryset).
That is apparently a case that was not thought of (why?). I thought it was somehow a bug, and that there is a design decision to take before fixing it.

Unless you cannot know in advance which class you'll be using at the start of the call

That's exactly why I had this issue :)

if you want to get all Machine specialization you can do Machine.specializations.all()

Unless you have instances of Machine, then it will fail

CI.specializations.all() is nonsense

Why ? int is a subclass of int, so why not considering CI as a specialization of CI ?

Doesn't sound like an insane amount of work to me ... and I would have a preference for the solution 2, because it sounds like the cleanest to me. Or maybe you have a better idea ?

[gnarea]

I think it makes sense to consider CI.specializations == CI.objects given that it's a final specialization and therefore all its objects are specialized as "CI".

I also think it'd be OK to support instances of generalizations like Machine, as long as it's not been flagged as abstract in the model meta. At the moment we take all generalizations are abstract because all our generalizations are abstract in our code.

If you can think of solutions that are 100% backwards compatible and you're willing to implement them yourself following our conventions (e.g., code formatting, unit tests), I wouldn't see why these things couldn't be changed as you suggest. However, this is my opinion rather than a "go ahead" -- Let's see what Euan thinks.

[euangoddard]

I agree with Gustavo. sebpiq, if you want to submit a fix, then we'll certainly take a look. Please ensure there is full test coverage.

As an aside, I'm not sure I agree with python's issubclass function results for the same class. It seems counter-intuitive to me. Were there such a function as issuperclass, what should issuperclass(int, int) return?

[sebpiq]

Cool !

You can call me Sébastien :)

issubclass(int, int) == True never shocked me. It actually makes sense if you consider "issubclass" as testing a set inclusion. Would it be shocking if it was issubset("all subclasses of int", "all subclasses of int") == True ? And same, I wouldn't be shocked with issuperclass(int, int) == True.
Now, one Python good thing is its consistency, and I would say that even if you don't fully agree with a design decision made in Python, it's better to respect the conventions used, so the users can guess more easily how your code will behave ... So - if it was me - I would clearly consider each model as a specialization of itself.

This said, I'll implement that tomorrow, and hope that it'll convince you !!!

[franciscoruiz]

We are using djeneralize in production (I think that may be Euan's main concern), hence we have to freeze it before releasing. At that point we could consider adding these features to master and keep an open branch for maintenance until the new version becomes production-ready (in case they are backwards incompatible).

issubclass(A, A) == True can be misleading given that a class it's not an actual subclass of itself. The name may not be the best one but it's useful for practical purposes as it works consistently with the concept of polymorpish (one of the forms that A has is A itself, among the rest of its parents). That said, it would be useful to make a final specialization behave like a non-final one.

I also think it'd be OK to support instances of generalizations like Machine, as long as it's not been flagged as abstract in the model meta.

Totally agree.

[sebpiq]

Hey guys ! I can't run your tests ... Would be cool to add a guide in README. Do you need to install something else than nose, django-nose and fixture ?

I got this weird error :

Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/django/test/testcases.py", line 292, in call
self._pre_setup()
File "/usr/local/lib/python2.7/dist-packages/django/test/testcases.py", line 259, in _pre_setup
self._fixture_setup()
File "/usr/local/lib/python2.7/dist-packages/fixture/django_testcase.py", line 25, in _fixture_setup
if connection.creation._rollback_works():
AttributeError: 'DatabaseCreation' object has no attribute '_rollback_works'

Pb with a version of something ? Any idea ?

[euangoddard]

That rings a bell. I think it's a bug in the way Django uses sqlite DBs for testing. As I recall, you need to touch the DB file and then do

python manage.py syncdb

It doesn't make any sense since the test runner should create a whole new set of tables, etc., but it is the only way I found to get it to work.

[sebpiq]

Did the first command get swallowed ? python ... ? If it really doesn't work, I'll just reorganize the tests, then leave the fun to you - if you want to keep your way of testing - when you'll have to merge the tests.

[euangoddard]

Hmm. Looks like gut flavoured markdown doesn't work as discussed in the documentation. Should have previewed it.

I don't understand what you mean by "your way of testing"?

[sebpiq]

Well ... looks like the problem comes with using "fixture", so I'll just move all the tests in the django project, in different apps, and stop using fixture. I just wanna have the tests working, so I can start ; and if you don't like the way I re-layed-out the tests, you can roll back and merge my changes ... How does that sound ?

[franciscoruiz]

Hi Sébastien,

That error is already fixed in the latest code of fixture. You can get it from http://code.google.com/p/fixture/ (it's up to you).

[sebpiq]

arg ... Too late !

[sebpiq]

Ok ... done, there was actually very very little to refactor in the code. The tests were the hard part, so I restructured them a bit, I think it's much better like that (all tests are in a single django project, and I use django-nose to run them), but of course you can roll them back (including my modifs) very easily.

I'll go on deploy it on my projects where it was needed, see if it fixes my problems, and will probably have to fix again, and add a couple of tests before pull-requesting.

Thanks for the help !