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

Custom models #42

Closed
wants to merge 3 commits into from
Closed

Conversation

benjaoming
Copy link
Contributor

This fixes #28

Collect all settings in blog.settings, add simple documentation notes for settings (notice Sphinx syntax, we should add sphinx for this project soon)

Thanks bigtime to @Fantomas42 for the approach used in django-blog-zinnia for loading custom models.

@benjaoming
Copy link
Contributor Author

benjaoming commented Mar 20, 2017

This is still WIP.. gonna try to solve the odd setuptools failure in tests.

requirements.txt Outdated
@@ -7,3 +7,4 @@ django-contrib-comments==1.7.3
django-comments-xtd==1.6.3
requests
lxml
six
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we using six for? Is it really needed?

"""
A context manager that temporarily sets a setting and reverts to the original value when exiting the context.
"""
return blog_override_settings(**kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain more about what this is doing? Overriding settings I suppose? But I don't see it used elsewhere in the test.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I force-pushed another commit, but see my comments in #43

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper fix is in a351dcf since it's actually caused by Travis running pip 6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, Github was confused by my force-push there.. first time I ever tried anything weird on pip. I was responding to your comment on previous, overwritten commit:

What are we using six for? Is it really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain more about what this is doing? Overriding settings I suppose? But I don't see it used elsewhere in the test.py.

Yes, it's called by self.settings many places.

It's necessary to reload blog.settings when changing django settings. This is a common pattern because the override_settings context manager only changes and reloads django.conf.settings

@benjaoming
Copy link
Contributor Author

benjaoming commented Mar 22, 2017

After trying to implement this and playing around with various proxy and abstract models, I've concluded that it's not possible to implement this without renaming either the model which to access (the one proxying for custom settings) or renaming/moving the default database table and its accessor model (because otherwise it will name collide with the proxy model).

To put it shortly, we can't have a concrete model blog.BlogPage, inheriting from blog.BlogPageBase and then change blog.BlogPage to inherit from myapp.BlogPage - this would require a customized setup to add migrations to blog whenever the model is customized.

If we want to use blog.BlogPage as a proxy model with a configurable base, this means renaming the actual default models originally shipped as blog.BlogPage to something else like blog.BlogPageDefault and make all the FKs configurable, editing all the existing migrations.

So what I came up with is to not reinvent the wheel and instead opt for this:

https://pypi.python.org/pypi/swapper

Because Django's swappable API is considered undocumented and subject to change (although it hasn't changed for quite some time), it's probably safer to go through an application that implements whatever Django uses internally.

Closing this and trying a new PR with swapper instead...

@benjaoming benjaoming closed this Mar 22, 2017
@benjaoming
Copy link
Contributor Author

I've edited my explanation a bit, but feel free to ask!

@benjaoming
Copy link
Contributor Author

Just another quick update: I'm still working on this. It looks possible!

However, as with django.crontrib.auth, there might be a pitfall in the way to introduce a swappable model... i.e. you don't just swap it in the middle of something :) So swappable will continue to be a slightly misleading term :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants