Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Extending Entry model using the abstract entry model classes not possible #219

Closed
bikeshedder opened this Issue Mar 25, 2013 · 19 comments

Comments

Projects
None yet
3 participants

The Entry model is defined in the same module as the abstract entry model classes. This makes it impossible to create a custom Entry based on those abstract classes.

The documentation is also wrong as that zinnia.models.EntryAbstractClass does not exist anymore: http://docs.django-blog-zinnia.com/en/latest/how-to/extending_entry_model.html

The easiest fix for this would be to move the abstract entry model classes into a different module like zinnia.models_bases. Django Shop does it that way: https://github.com/divio/django-shop/blob/master/shop/models_bases/__init__.py

Update: An example project which triggers this problem can be found under https://github.com/bikeshedder/zinnia-custom-entry-example

@bikeshedder bikeshedder added a commit to bikeshedder/django-blog-zinnia that referenced this issue Mar 25, 2013

@bikeshedder bikeshedder Move abstract model classes to zinnia.models_bases
Fixes #219
1859922

@bikeshedder bikeshedder added a commit to bikeshedder/django-blog-zinnia that referenced this issue Mar 25, 2013

@bikeshedder bikeshedder Move abstract entry model to zinnia.models_bases.entry
Fixes #219
4ec6c58
Owner

Fantomas42 commented Apr 2, 2013

Hi,

first of all the documentation seems up-to-date, by specifying to inherit from
_from zinnia.models.entry import EntryAbstractClass_
Maybe you use an old version of the documentation.

Secondly I have just made the test of extending the Entry model following the documentation and everything works like a charm.

So I will close the issue.

@Fantomas42 Fantomas42 closed this Apr 2, 2013

The issue is a circular dependency that is bound to fail or at least cause troubles.

I prepared an example project which shows what is going wrong:
https://github.com/bikeshedder/zinnia-custom-entry-example

$ ./manage.py validate
/home/bikeshedder/.virtualenvs/zinnia-custom-entry-example/src/django-blog-zinnia/zinnia/models/entry.py:500: RuntimeWarning: zinnia_ext.entry.ExtEntry cannot be imported
  RuntimeWarning)

0 errors found

That warning is obviously an error and should be treated as such. Using the same code and the django shell you can see that it is perfectly possible to import zinnia_ext.entry.ExtEntry:

$ ./manage.py shell
/home/bikeshedder/.virtualenvs/zinnia-custom-entry-example/src/django-blog-zinnia/zinnia/models/entry.py:500: RuntimeWarning: zinnia_ext.entry.ExtEntry cannot be imported
  RuntimeWarning)

Python 2.7.3 (default, Sep 26 2012, 21:51:14) 
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from zinnia.models import Entry
>>> from zinnia_ext.entry import ExtEntry
>>> issubclass(Entry, ExtEntry)
False

zinnia_ext.entry requires zinnia.models.entry which requires zinnia_ext.entry ... a circular import. Once that "Warning" has been emitted importing both modules will work but the Entry class now uses the wrong base class. Besides fixing this issue I'd also like to propose changing this warning to an error as it is very unlikely that anyone wants to run zinnia with the default entry base class if ZINNIA_ENTRY_BASE_MODEL has been specified in the settings.

In the Documentation at http://docs.django-blog-zinnia.com/en/latest/how-to/extending_entry_model.html you even made it clear that the Entry model must not be imported: "Do not import the Entry model in your file defining the extended model because it will cause a circular importation."

In order to inherit from the abstract entry classes one must import the zinnia.models.entry module which causes exactly this "circular importation". The only solution for this is to separate the abstract base classes from the Entry class.

If you are now able to reproduce this issue I'll rebase my changes and create a new pull request.

Owner

Fantomas42 commented Apr 3, 2013

Using the correct import syntax, this will work.

https://gist.github.com/Fantomas42/a25d62a3b71dbdbe13dc

The import part is more verbose, but cleaner IMO.

It depends on the order of imports, too. See my comment in the gist: https://gist.github.com/Fantomas42/a25d62a3b71dbdbe13dc#comment-809975

Owner

Fantomas42 commented Apr 3, 2013

How the order of importation could change ???
Even if you swap the application order in INSTALLED_APPS, it doesn't change anything.

In this case, it's not a question of robustness, it's a question of following the documentation by using the good importation syntax.

Regards

apollo13 commented Apr 3, 2013

Relying on a special import (or import order) to get working code is hardly a good choice. There is no such thing as "good importation syntax". All import variants in python should be considered equal -- if that is not the case it's imo not an issue of the enduser but of the application (in this case zinnia) itself.

Owner

Fantomas42 commented Apr 3, 2013

All import variants in python should be considered equal -- if that is not the case it's imo not an issue of the enduser but of the application (in this case zinnia) itself.

I agree that all import variants in Python should be considered equal, but when you do

from zinnia.models import entry as ze

it's not equal to

from zinnia.models.entry import something

3 rules are defined to extend the Entry model, the first rule tells to not import Entry, that's the first example syntax exactly do.

You are free to not respect the rules, but don't be surprised if the things does not work as expected.

Relying on a special import (or import order) to get working code is hardly a good choice.

What is the best solution ? Use the undocumented Meta.swappable attribute to do the extension ? Or create a new mess like suggested by putting the abstract classes in a new sub-module, just for a question of importation in a single line ?

For me, everything works fine if you just follow the rules.

Regards

apollo13 commented Apr 3, 2013

I agree that all import variants in Python should be considered equal, but when you do

from zinnia.models import entry as ze

it's not equal to

from zinnia.models.entry import something

but

from zinnia.models import entry as ze
ze.something

Should be considered equal to your second form.

3 rules are defined to extend the Entry model, the first rule tells to not import Entry, that's the first example syntax exactly do.

No, The first example import the entry module, although as soon as models.__init__ is read Entry is imported anyways…

Or create a new mess like suggested by putting the abstract classes in a new sub-module, just for a question of importation in a single line ?

How is a clear structure a mess, especially if it solves otherwise hard to debug errors.

For me, everything works fine if you just follow the rules.

Rules set up by you which shouldn't need to exist if you ask me.

Owner

Fantomas42 commented Apr 3, 2013

Should be considered equal to your second form.

Yes but in one case we import a module, but in the other we import a class ? How can it be equal ?
I understand well that Entry is imported when you import the entry module, that's why I don't suggest to do it.

How is a clear structure a mess, especially if it solves otherwise hard to debug errors.

It's a mess when you need to do extra works, for someone who don't want to follow the documentation. This system has worked perfectly until now. For me it's clearer to have the models in the submodule named models even if the models are composed of abstract classes. And nothing it's hard to debug, when I know what you do.

Rules set up by you which shouldn't need to exist if you ask me.

If I wouldn't have set these rules, this feature would not exist.
We all make OSS, so if my rules doesn't fit to you, you are free to fork and build your own rules, but be caution they may not fit to another person.

Regards

apollo13 commented Apr 3, 2013

Yes but in one case we import a module, but in the other we import a class ? How can it be equal ?

Both import just the module and construct it in sys.modules, the "from bla import x" is merely a shortcut to get a reference to something in the module.

I understand well that Entry is imported when you import the entry module, that's why I don't suggest to do it.

That makes absolutely no sense.

It's a mess when you need to do extra works, for someone who don't want to follow the documentation.

There shouldn't be any need to follow documentation for something that should just work.

If I wouldn't have set these rules, this feature would not exist.

What? The rules are just due to an limitation you impose on yourself, the feature could exist anyways (the pullrequest exactly proves that). -- so please don't argue like that.

We all make OSS, so if my rules doesn't fit to you, you are free to fork and build your own rules, but be caution they may not fit to another person.

As a developer I obviously try to not fork everything along the way to generate further fragmentation but try on improving existing stuff -- but yeah you are right, if moving a module around is already to much for you forking seems like the only way (sadly).

Owner

Fantomas42 commented Apr 3, 2013

That makes absolutely no sense.

I agree, but how can it works with my version if no differences are involved ?

What? The rules are just due to an limitation you impose on yourself, the feature could exist anyways (the pullrequest exactly proves that). -- so please don't argue like that.

And I don't see the advantage of this pull request, excepting for the import part. So as a (lazy) developer moving this part in another module make no sense for me.

Owner

Fantomas42 commented Apr 3, 2013

If only I could see the advantage of this pull request, I will merge it, however at the present time, I see only errors related to a lack of reading the documentation.

apollo13 commented Apr 3, 2013

I agree, but how can it works with my version if no differences are involved ?

It doesn't work with your version either :þ

See the following (with your correction-"patch" applied):

>>> from zinnia.models import Entry
>>> from zinnia_ext.entry import *
>>> issubclass(Entry, ExtEntry)
True

Entry inherits from ExtEntry, everthing is fine, this works cause:

  • from zinnia.models import Entry triggers the import of zinnia_ext.entry
  • zinnia_ext.entry imports zinnia.models.entry which is okay, cause at this time zinnia.models.entry is already fully (99% the Entry object is still under construction, but this doesn't hurt here) initialized.

Now compare with this import order (again, your correction patch applied) [in a new shell!]:

>>> from zinnia_ext.entry import ExtEntry
/home/florian/.virtualenvs/ca0f0a5b83eb51f5/src/django-blog-zinnia/zinnia/models/entry.py:500: RuntimeWarning: zinnia_ext.entry.ExtEntry cannot be imported
  RuntimeWarning)

>>> from zinnia.models import Entry
>>> issubclass(Entry, ExtEntry)
False
>>> 

So from zinnia_ext.entry import ExtEntry directly triggers a warning and Entry is not a subclass of ExtEntry; here is why:

  • if you import zinnia_ext.entry it tries to import all the abstract classes from zinnia.models.entry, this also causes the zinnia.models.entry module to be fully initialized.
  • When the full initialization happens, zinnia.models.entry.Entry tries to import zinnia_ext.entry.ExtEntry which will fail cause ExtEntry is not yet constructed (the interpreter is still in the first import statement), so you already have zinnia_ext.entry in sys.modules but there is no ExtEntry attribute yet.

I hope I made it clear what we were trying to say when we were refering to import error issues. If not please say what's unclear and where.

Owner

Fantomas42 commented Apr 4, 2013

Now compare with this import order (again, your correction patch applied) [in a new shell!]:

>>> from zinnia_ext.entry import ExtEntry
/home/florian/.virtualenvs/ca0f0a5b83eb51f5/src/django-blog-zinnia/zinnia/models/entry.py:500: RuntimeWarning: zinnia_ext.entry.ExtEntry cannot be imported
  RuntimeWarning)

>>> from zinnia.models import Entry
>>> issubclass(Entry, ExtEntry)
False

This is really strange, because no errors are raise in my side

>>> from zinnia_ext.entry import ExtEntry
>>> from zinnia.models import Entry
>>> issubclass(Entry, ExtEntry)
True

@Fantomas42 Fantomas42 reopened this Apr 4, 2013

apollo13 commented Apr 4, 2013

What python version? Try with -Wall, might help you (or replace the warning stuff with print for testing)

Owner

Fantomas42 commented Apr 4, 2013

Python 2.7.3 (default, Aug 1 2012, 05:16:07)
[GCC 4.6.3] on linux2

The -Wall argument or replace the warning by a print, does not change the result.

apollo13 commented Apr 4, 2013

And you are using a fresh shell and not manage.py shell? You don't have any customizations which loads (pythonrc etc) which preloads models, really just a plain python shell?

If the answer to that is yes, then I have no idea why it works for you, it really shouldn't.

Owner

Fantomas42 commented Apr 4, 2013

Without using manage.py shell I can reproduce the issue.

It's enough for me to consider that a fix is needed.

Thanks for your help

apollo13 commented Apr 4, 2013

Thank you! With manage.py you don't run into this cause it has the same effect as importing zinnia.models before the extension.

@Fantomas42 Fantomas42 closed this in f684f9f Apr 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment