Session/cookie based messages (#4604)
Tobias
start a conversation about cleaning up session messages enough to get
them into core or contrib. Alex suggested that the ticket (http://
code.djangoproject.com/ticket/4604) was not the right place for the
conversation so I'm starting one here.
There are a number of usable solutions out there but django-notify is
the most complete, polished one that I know of. I just contributed a
patch for combo/fallback storage, a version of which SmileyChris
integrated into trunk (of django-notify, not django). It's a solid
product with well-commented code and a good test suite.
What needs to be done to get this or something like it into contrib?
Tobias
Hanne Moa
> There are a number of usable solutions out there but django-notify is
> the most complete, polished one that I know of.
I've just tested django_notify, so far it seems plenty good enough to
replace all my pass-on-message hacks (not to mention being much more
elegant than any of 'em) expect for not being able to set a message
php-style in the GET, this way:
http://example.com/?msg=Shiver+me+timbers
Whether being able to do that at all is desirable is another question
entirely ;)
Big bonus in my view: it's completely independent of the
django.contrib.auth-system.
> What needs to be done to get this or something like it into contrib?
Yes, this inquiring mind also wants to know.
HM
Russell Keith-Magee
Since you were evidently at my DjangoCon presentation, you know that I
(and the core in general) have a pretty high level of reticence to
adding things to contrib - especially when the package can live quite
happily outside of core. So far, everything I have seen about
django-notify/djangoflash makes it seem like a completely standalone
package that will gain nothing from being part of core, other than
increasing the size of the tarball, and increasing the maintenance
load of the core team. One of the questions that needs to be answered
is "why should this be shipped with Django?"
Packages also need to meet the basic definition of django.contrib in
order to be accepted. Part of this definition is that it must be the
defacto standard implementation. In order to be the defacto standard
implementation of anything, it needs to be relatively stable and
feature complete. When you make statements like "I just contributed a
patch for combo/fallback storage", it leads me to believe that this
may not be the case - at least, not yet.
You also mention that there are a number of other implementations, but
you haven't really given a compelling survey or analysis of the
alternatives - you've just blessed one in particular. Why?
Another piece of the puzzle that is missing from my perspective is any
discussion of how session-based messaging interacts with the existing
contrib.auth messaging framework. IMHO, Django shouldn't ship with two
messaging APIs, so what is the integration point? Do we need a
messaging backend API? I haven't thought about this problem anywhere
near enough to offer a proposal, but I haven't seen any discussion of
this point either. A good solution for this problem would go a long
way to greasing the wheels.
Yours,
Russ Magee %-)
Chris Beaven
> Another piece of the puzzle that is missing from my perspective is any
> discussion of how session-based messaging interacts with the existing
> contrib.auth messaging framework. IMHO, Django shouldn't ship with two
> messaging APIs, so what is the integration point?
sense; it's a very common usage pattern.
Attaching these kind of messages to a User instance is wrong: there is
not an enforced one to one correlation between a user and a session
[1], and you can't notify anonymous sessions.
The current contrib.auth messaging framework can't be removed or
replaced while keeping full backwards compatibility. A basic user
based messaging system still has it's uses, but it is not the best fit
for session notifications.
Side note: a pet peeve of mine is that contrib.auth messages are not
lazily loaded, which means you get a db hit every request [when using
a RequestContext] and lose your messages whether you use the messages
context var or not, but this is a side issue and should probably be a
ticket of it's own.
For some background, I initially started 4604 much more tightly
coupled with core, and one which backwards-compatibly worked with the
current user-based messaging system. Malcolm specifically requested
that this should be written as a stand-alone contrib app rather than
part of core, and that it should have minimal impact on existing code.
Russell Keith-Magee
Sorry - perhaps I need to be more clear on my intent here. I'm
convinced of the importance of session-based messages - just not of
when and how a particular implementation should be added to trunk.
I agree that they are a useful idea. I agree with Malcolm that session
based messages should be a standalone app. I don't think they should
be tightly bound to core.
What I'm talking about is an orthogonal set of modifications that
would allow for _any_ messaging system (database backed, session
backed, or magic pony backed) to be used. This doesn't couple a
particular implementation of session-based messages to the core, but
it would allow for end-users to choose a session-based message
framework if they found one that was appropriate to their needs. In
the fullness of time, we may end up adding django-notify or similar to
contrib, but if we add the plugable backend we can defer that decision
until much later when implementations have stabilized and a clear
implementation winner has emerged.
If such an abstraction API isn't possible (and the differences between
the capabilities of the two certainly makes this likely), then that
makes it even more important that we choose the right implementation
to add to contrib. As it stands, I'm aware of at _least_ two
implementations (django-notify and djangoflash), but I have no real
feel for the implementation maturity or general level of community
acceptance of either. I'm not going to advocate adding anything to
contrib until I can be convinced that one particular implementation
really is the defacto standard implementation of the pattern.
Yours,
Russ Magee %-)
Chris Beaven
On Sep 21, 12:05 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
> What I'm talking about is an orthogonal set of modifications that
> would allow for _any_ messaging system (database backed, session
> backed, or magic pony backed) to be used. This doesn't couple a
> particular implementation of session-based messages to the core, but
> it would allow for end-users to choose a session-based message
> framework if they found one that was appropriate to their needs. In
> the fullness of time, we may end up adding django-notify or similar to
> contrib, but if we add the plugable backend we can defer that decision
> until much later when implementations have stabilized and a clear
> implementation winner has emerged.
It comes with packaged with three backends, and custom backends are
also supported.
> As it stands, I'm aware of at _least_ two
> implementations (django-notify and djangoflash),
To clarify, django flash is actually more than just a session
notification backend, it introduces a whole new temporary (but cross-
request) variable scope.
Leah has a messaging implementation which uses django-flash:
http://github.com/leah/django-flash-status
So the question changes into, "are we after just a notification system
or do we want to have a more open (but arbitrary) system of a
temporary cross-request variable scope?"
Alex Gaynor
sessions or cookies?
Alex
> >
>
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Tobias McNulty
Having some kind of defacto cross-request notification system makessense; it's a very common usage pattern.
Attaching these kind of messages to a User instance is wrong: there is
not an enforced one to one correlation between a user and a session
[1], and you can't notify anonymous sessions.
Tobias McNulty
You also mention that there are a number of other implementations, but
you haven't really given a compelling survey or analysis of the
alternatives - you've just blessed one in particular. Why?
veena
good idea with start a wiki page.
I'm not sure if we don't forget one issue.
How about same session (or same cookie sent by browser) with
simultaneously opened windows of one browser? Then message could
appear in different window not the right one where we invoke the
event. Is it a problem? Is only possibility to get of this issue that
flash app should add a query parameter into redirected url which would
identify the right window?
Vaclav
On Sep 22, 3:08 am, Tobias McNulty <tob...@caktusgroup.com> wrote:
> On Sun, Sep 20, 2009 at 10:24 AM, Russell Keith-Magee <
>
Tobias McNulty
Russell Keith-Magee
> Vaclav,
> I think this is less of an issue, because you'd have to switch to another
> tab and perform a second operation that generated feedback in the ~200
> millisecond window of time between clicking a link and the new page loading.
Ah... the wonderful American perspective of the internet. :-)
As a proud resident of the antipodes, allow me to assure you that
200ms is not a representative sample of the time required to load a
page for those of us that don't live in the continental USA.
Let's consider Rackspace as a representative sample of a US based
server with plenty of bandwidth. As the crow flies, I'm about 17000 km
from a server in Texas. By simple laws of physics, it takes 60ms for
the signal to get from me to the server, and 60ms for the signal to
get back. That's 120ms, and I'm not even taking into account:
* The time spent going through the 14 routing points between me and
Rackspace's servers
* The fact that the cable between me and Texas doesn't follow the
same route as a crow
In reality, I get a ping time closer to 300 ms. And that's to a
high-end data center under ideal conditions - it can be much larger if
I'm dealing with low end providers.
Now add the time required to actually compute a response.
To put this in practical perspective - during DjangoCon, I heard lots
of people complaining about the speed of the hotel wireless network. I
didn't notice a serious change from what I use at home every single
day.
In this particular case, I think you're correct - I'm not especially
concerned about parallel cookie requests. However, there are plenty of
arguments that get made on the assumption that the internet is a form
of instant communication (<cough> database connection lag </cough>). I
want to drive home the point that Earth is a very large place, and no
matter where you are located, most of the world's population isn't
anywhere near you - and no amount of technology will fix problems
caused by the laws of physics.
Yours,
Russ Magee %-)
Tobias McNulty
In reality, I get a ping time closer to 300 ms. And that's to a
high-end data center under ideal conditions - it can be much larger if
I'm dealing with low end providers.
What?? 200 ms is the average quoted by Mr. Sproutcore himself!
No, in all seriousness, my apologies for making such a broad generalization about packet latency. I could/should have said 500 ms or even a second; the argument and corresponding solution, if needed, remain the same.
Anyways..we digress. I am marking this a non-issue in my own head - please pipe up if there's a case to be made.
Tobias
David Cramer
(even tho we did this about a year ago), but recently just OS'd our
simply notices system [1]. I'm also +1 for including something like
this in trunk rather than using the current user messages. I had a
brief look at django_notify and its a similar approach, but it seems
much more configurable than what we wrote.
[1] http://github.com/dcramer/django-notices
On Sep 22, 9:49 pm, Tobias McNulty <tob...@caktusgroup.com> wrote:
> On Tue, Sep 22, 2009 at 9:51 PM, Russell Keith-Magee <freakboy3...@gmail.com
Tobias McNulty
significant changes to the wiki page that try to sum up the state of
the conversation so far in a better way:
http://code.djangoproject.com/wiki/SessionMessages
Things that still need to be discussed/done:
* Coming to consensus on what 3rd party app we actually choose to
extend/modify to fit into Django
* What to do with the existing user message API (see
http://code.djangoproject.com/wiki/SessionMessages#IntegratingwiththeExistingAPI)
* Review/add to the TODO list on the wiki
(http://code.djangoproject.com/wiki/SessionMessages#TODOOnceaSolutionisChosen)
Cheers,
Tobias
--
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com
Tobias McNulty
> Things that still need to be discussed/done:
>
> * Coming to consensus on what 3rd party app we actually choose to
> extend/modify to fit into Django
>
> * What to do with the existing user message API (see
> http://code.djangoproject.com/wiki/SessionMessages#IntegratingwiththeExistingAPI)
>
> * Review/add to the TODO list on the wiki
> (http://code.djangoproject.com/wiki/SessionMessages#TODOOnceaSolutionisChosen)
I should have also added:
* Coming to consensus on a de facto standard API suitable for
inclusion in Django
I originally put it on the TODO list, but in retrospect the discussion
needn't wait till we pick a solution.
As a practical starting point, I copied the API for django-notify to
the wiki page. I like the general form of that API, but I'd be
slightly more happy with something like:
from django.contrib import messages
request.messages.add('message', messages.INFO)
# or
request.messages.add('message', classes=(messages.WARNING,))
# or
request.messages.error('message')
A la python logging, I think endorsing a specific set of message
classes or tags (but still allowing them to be extended) is A Good
Thing because it helps reusable apps provide more customized feedback
in a standard way.
David Cramer
Sent from my iPhone
On Oct 10, 2009, at 12:53 PM, Tobias McNulty <tob...@caktusgroup.com>
Tobias McNulty
> The proposal per your email is more or less how django-notices works.
For comparison I added django-notices to the Available Options:
http://code.djangoproject.com/wiki/SessionMessages#AvailableOptions
Hanne Moa
> I should have also added:
>
> * Coming to consensus on a de facto standard API suitable for
> inclusion in Django
The suggested API: how hard will it be to use different message-types
in addition to the syslog-like ones? I use django-notify now and the
types I use is nothing (for info), "error", and "help" (for
help/tips). I've also made django-notify optional in my reusables,
with a dummy-object if it is not installed. Locking them down with
class.attributes will find typos of course, but right now I don't have
to import anything just to use session-messages, I just do a
hasattr(request, 'notifications') .
HM
Tobias McNulty
> The suggested API: how hard will it be to use different message-types
> in addition to the syslog-like ones?
I think it should definitely be easy, if not actively encouraged. IMO
those constants should just be strings, or integers which can easily
be mapped to strings (like in Python logging). This would give you
the flexibility of passing in anything you want. django-notices does
it the latter way:
http://github.com/dcramer/django-notices/blob/master/django_notices/constants.py
I clarified that criterion in the wiki:
http://code.djangoproject.com/wiki/SessionMessages#Criteria
veena
seconds to load a page.
I save in admin in two tabs and got a notice in one tab from the other
tab.
But I agree, I defer this "bug" for the first release od django
messaging. I think django isn't now in right mood to add there some
functionality like adding of query parameters to response object by
some application. Maybe in future.
Vaclav
On Sep 23, 4:49 am, Tobias McNulty <tob...@caktusgroup.com> wrote:
> On Tue, Sep 22, 2009 at 9:51 PM, Russell Keith-Magee <freakboy3...@gmail.com
Tobias McNulty
> Today I was on very bad wifi connection. Constantly dropped. 20
> seconds to load a page.
> I save in admin in two tabs and got a notice in one tab from the other
> tab.
>
> But I agree, I defer this "bug" for the first release od django
> messaging. I think django isn't now in right mood to add there some
> functionality like adding of query parameters to response object by
> some application. Maybe in future.
AFAIK this will become a non-issue with the advent of HTML5, as each
window/tab will have its own session.
Tobias
David Cramer
pass messages to the context, but you would lose the ability to
retrieve those variably. I believe storing it in the existing session
is the best appoach for django's builtin support.
Sent from my iPhone
On Oct 10, 2009, at 8:00 PM, Tobias McNulty <tob...@caktusgroup.com>
wrote:
>
Tobias McNulty
> I also don't think this problem is being addressed here. Yes you could
> pass messages to the context, but you would lose the ability to
> retrieve those variably. I believe storing it in the existing session
> is the best appoach for django's builtin support.
I'm not exactly sure what problem you see as not being addressed, and
I don't think you were arguing against this, but it has been noted
elsewhere that it's more desirable to store the messages directly in a
cookie, if possible, to avoid the extra database or cache hit.
It seems well worth it to me and there are solutions out there that
try to store the messages in a cookie and then fall back to the
session for longer (>4kb) messages.
Paul McLanahan
For me, the best solution would be a combination of django-notify and
django-notices. django-notify for their fallback backend and the
interchangeable backend support in general, and django-notices for the
logger-like API. I especially like the NOTICE_LEVEL setting which
could easily be controlled via your DEBUG setting. I agree that custom
levels should be easy and encouraged, and the strings used even for
the built-in levels should be configurable.
Paul
Tobias McNulty
> For me, the best solution would be a combination of django-notify and
> django-notices. django-notify for their fallback backend and the
> interchangeable backend support in general, and django-notices for the
> logger-like API. I especially like the NOTICE_LEVEL setting which
> could easily be controlled via your DEBUG setting. I agree that custom
> levels should be easy and encouraged, and the strings used even for
> the built-in levels should be configurable.
I'm with you 100%. The criteria on the wiki page say as much, but I
should probably clarify that django-notify does not actually provide
any default levels/classes/tags yet. Thanks for the feedback.
David Cramer
As using cookies to send messges is fine if you guarantee the entity
is removed onthe next hit. Otherwise it becomes a performance issue
storing them for longer periods of time.
While I don't think that is common it should definitely be a factor in
the decision.
Sent from my iPhone
On Oct 12, 2009, at 9:39 AM, Tobias McNulty <tob...@caktusgroup.com>
wrote:
>
Paul McLanahan
http://code.djangoproject.com/wiki/SessionMessages#PotentialAPI
It would be really nice if one could optionally iterate over the
message lists separately. For example:
{% if request.messages.error %}
<ul class="messages error">
{% for message in request.messages.error %}
<li>{{ message }}</li>
{% endfor %}
</ul>
{% endif %}
And then get the rest normally...
{% if request.messages %}
<ul class="messages">
{% for message in request.messages %}
<li{% if message.tags %} class="{{ message.tags }}"{% endif
%}>{{ message }}</li>
{% endfor %}
</ul>
{% endif %}
This would allow for much more flexibility from a design perspective.
Paul
Tobias McNulty
> I would definitely say it needs to be configurable then at the least.
> As using cookies to send messges is fine if you guarantee the entity
> is removed onthe next hit. Otherwise it becomes a performance issue
> storing them for longer periods of time.
>
> While I don't think that is common it should definitely be a factor in
> the decision.
django-notify lets you plug in a pure session storage if that's what you want.
And if you do use cookies, they'll get removed 90% of the time on the
next request, and in most other cases removed on the subsequent
request (e.g., in the case of 2+ chained redirects).
The point is just to keep them around until they're actually
displayed, not indefinitely.
Tobias McNulty
> For the potential API...
>
> http://code.djangoproject.com/wiki/SessionMessages#PotentialAPI
>
> It would be really nice if one could optionally iterate over the
> message lists separately. For example:
I have no particular issue with allowing messages to be iterated in
full and/or by type. We could also just sort the messages and let you
use {% regroup messages by tag %}, but I suppose you want the ability
to pull out a particular type of error and that could get tedious with
{% regroup %}.
Paul McLanahan
> I have no particular issue with allowing messages to be iterated in
> full and/or by type. We could also just sort the messages and let you
> use {% regroup messages by tag %}, but I suppose you want the ability
> to pull out a particular type of error and that could get tedious with
> {% regroup %}.
I think regroup would be fine. My thought was that with iteration by
type you'd be able to easily highlight just one type if you so choose,
and even have any of the types displayed in a completely different
area of the markup if you needed. Having both abilities would be the
best-of-all-possible-worlds in my opinion, but this is starting to
smell like scope-creep. I'll just say that the backends and API are
the most important things, and having individually iterable lists is
just a "nice to have." I'd definitely be happy to help with the
implementation though if the group thinks it a worthy feature.
Paul
David Cramer
----
David Cramer
Tobias McNulty
> I'm -1 on adding .errors or whatever. The main reasoning being that "levels"
> are simply numeric values (even though django-notices does provide default
> labels for its built-ins). Regroup is very easy and I don't think this is
> something that needs built-in, as there's no better way to optimize it than
> regroup. However, with doing this it'd be very important that it doesn't
> clear the messages unless you're pulling it out. django-notices handles this
> by popping out elements in the iter (I believe), so that if you don't pop
> the message out, it's still present.
In Python logging the levels are integers, but you still get
convenience methods like logger.error('msg'), etc.
I don't see why a similar method to filter the messages by type would
be a bad thing.
For the record I have no personal need for this feature (and hence
strong feelings about it), but nor do I see why adding it would be a
bad thing IF the need is actually there. Right now it sounds like
regroup will work fine for its original proponent, so, in the interest
of keeping things simple, let's assume it will not be included unless
someone brings up a compelling case.
David Cramer
----
David Cramer
Paul McLanahan
> For the record I have no personal need for this feature (and hence
> strong feelings about it), but nor do I see why adding it would be a
> bad thing IF the need is actually there. Right now it sounds like
> regroup will work fine for its original proponent, so, in the interest
> of keeping things simple, let's assume it will not be included unless
> someone brings up a compelling case.
That's fair enough. Simple is good, and if someone really needs
different message retrieval methods, I'm sure a custom middleware
could provide anything they'd need.
Tobias: Are you planning on coding up a proposal app? It doesn't sound
like there's far to go from a branch or fork of django-notify. I
should have some time to contribute to the code or testing if you need
it.
Thanks for your work on this (and on django-notify),
Paul
Tobias McNulty
> Tobias: Are you planning on coding up a proposal app? It doesn't sound
> like there's far to go from a branch or fork of django-notify. I
> should have some time to contribute to the code or testing if you need
> it.
Thanks for offering up your time. Once there seems to be a general
consensus about where we're going I plan to do that, yes. We're
getting closer but I'm not convinced we're there, yet.
Before we get too far, I'd appreciate hearing feedback from one or
more of the core devs (and from some of the folks who were involved in
this discussion back when it was happening on the ticket) on the wiki
page[1] and the general direction this project is going.
Tobias
[1] http://code.djangoproject.com/wiki/SessionMessages
Chris Beaven
> Before we get too far, I'd appreciate hearing feedback from one or
> more of the core devs (and from some of the folks who were involved in
> this discussion back when it was happening on the ticket) on the wiki
> page[1] and the general direction this project is going.
evangelizing :P
I've just updated django-notify to use notification levels (similar to
django-notices) - not having standard levels was the only thing I felt
was missing from django-notify (unfortunately, this is an incompatible
change for people who were using it to add custom tags, but I felt it
was worth it for the sake of moving forwards)
One thought: It would be backwards incompatible to change
{{ messages }}, it's already being used for User messages. To avoid
confusion, I have consistently steered away from using the term
"messages" by itself in documentation and code. I see that the wiki
page doesn't make the same distinction.
Tobias McNulty
> One thought: It would be backwards incompatible to change
> {{ messages }}, it's already being used for User messages. To avoid
> confusion, I have consistently steered away from using the term
> "messages" by itself in documentation and code. I see that the wiki
> page doesn't make the same distinction.
Yes, that's a choice we'll have to make. Most folks seem to default to
calling them messages (or maybe that's just me).
That said, I see your point. What about {{ request.messages }}? Is
that still going to be confusing? Whatever the name becomes, I don't
really see the need for the extra context processor. You'll likely be
writing the name far more often in your views than in your templates.
This also raises the issue of how this app will live side by side with
User messages. I threw out a few ideas on the wiki (give User
messages a much narrower purpose than they have now and/or phase them
out altogether), but I'm sure there are other (probably better) ones.
My only real issue with "notifications" is that it's more letters to
type and fewer letters in the actual message (before the line hits 80
chars).
Tobias
Russell Keith-Magee
>
> Before we get too far, I'd appreciate hearing feedback from one or
> more of the core devs (and from some of the folks who were involved in
> this discussion back when it was happening on the ticket) on the wiki
> page[1] and the general direction this project is going.
On the whole, I'm comfortable with what I'm seeing. I haven't looked
into the codebase for django-notify, but the feature set certainly
makes it look like a reasonable starting point.
Notwithstanding a teardown of the actual code, I have three comments
on progress thus far:
Firstly, given the similarity between the APIs, it seems like we
should be trying to maintain parity with logging. That means:
request.messages.add(messages.INFO, 'message')
with:
request.messages.info('message')
as an alias. Note that this is a reversal of argument order from the
proposed API. I'm not absolutely tied to this suggestion - I'm just
noting that adding Django support for Python logging is also on the
cards for v1.2, and it seems weird that we would introduce two APIs
with similar external APIs, but not try to maintain parity.
Secondly, I share Chris' concern about overloading 'messages'. This
overlaps with the bikeshed issue of what to call the new contrib
package - whatever name is picked for the contrib package should
probably be reflected in the context variable, etc.
Lastly, one issue that seems unaddressed is the transition plan for
replacing user.message_set. The preamble to the wiki page makes a
compelling argument for ditching message_set, but remains silent on a
proposal for how Django (as a project) should be treating this new
alternative.
The solution might well be "use it if you want, ignore it if you
don't", but given the problems with user.message_set, it might be
worth giving some thought to a deprecation/transition plan.
Yours,
Russ Magee %-)
Hanne Moa
<freakb...@gmail.com> wrote:
> I'm just
> noting that adding Django support for Python logging is also on the
> cards for v1.2, and it seems weird that we would introduce two APIs
> with similar external APIs, but not try to maintain parity.
In fact, it would be very useful to both log and add a message at the
same time, iff there is an error. So that shouldn't deliberately be
made hard to do but maybe suitable for a lazy man's shortcut: one call
that does both.
HM
David Cramer
----
David Cramer
Tobias McNulty
> In fact, it would be very useful to both log and add a message at the
> same time, iff there is an error. So that shouldn't deliberately be
> made hard to do but maybe suitable for a lazy man's shortcut: one call
> that does both.
That is binding together two technologies that are still fairly
conceptual in terms of their integration with Django at this point.
I'm a little worried that we wouldn't get it right the first time
around. The extreme case would involve implementing messaging as a
python logging handler. Frankly I'm not sure it's worth it.
Furthermore, "error" message to the user may not always be, in fact
often probably isn't, an error for the web app, so it'd be important
to differentiate between the two. E.g., you might want to log an
ERROR message under the INFO python logging level.
Short of providing this do-all call in the implementation itself, I
see a few options:
1) write your own wrapper if you do a lot of that
2) add some logging calls to the messaging code so that you can see
messages that were shown to the user if you want to, under a static
logging level (e.g., INFO)
I like #2, provided that the default logging handler in Django is a
NullHandler (I think it is/will be).
Tobias
Jacob Kaplan-Moss
<freakb...@gmail.com> wrote:
> Lastly, one issue that seems unaddressed is the transition plan for
> replacing user.message_set. The preamble to the wiki page makes a
> compelling argument for ditching message_set, but remains silent on a
> proposal for how Django (as a project) should be treating this new
> alternative.
This is my main concern, and the thing keeping me from being
enthusiastic about the current state of the proposal. Many apps --
including, notably, django.contrib.admin -- rely on user.message_set.
We can't simply remove it; we need to follow a deprecation schedule.
But we also shouldn't just keep two parallel "messaging"
implementations; that's silly.
The best solution, I think, would be to implement user messages
(user.message_set, get_and_delete_messages, etc) in terms of the new
framework, and then gradually deprecate and remove the old APIs.
Jacob
Hanne Moa
> On Tue, Oct 13, 2009 at 3:53 AM, Hanne Moa <hann...@gmail.com> wrote:
>> In fact, it would be very useful to both log and add a message at the
>> same time, iff there is an error. So that shouldn't deliberately be
>> made hard to do but maybe suitable for a lazy man's shortcut: one call
>> that does both.
>
> That is binding together two technologies that are still fairly
> conceptual in terms of their integration with Django at this point.
>
> I'm a little worried that we wouldn't get it right the first time
> around. The extreme case would involve implementing messaging as a
> python logging handler. Frankly I'm not sure it's worth it.
I think y'all misunderstood what I tried to say. I think it'll be
common to "bundle" them like this (it's not as if we can prevent
anyone from doing it!) and that there might one day be a desire for an
official shortcut to do it.
> Short of providing this do-all call in the implementation itself, I
> see a few options:
>
> 1) write your own wrapper if you do a lot of that
Already have, for django-notify. Since I already need to test that a
request has a "notifications"-attribute I have a handy place to attach
other "messaging" in the wide sense too, like logging, debug-stuff,
assert False when manually checking things...
> 2) add some logging calls to the messaging code so that you can see
> messages that were shown to the user if you want to, under a static
> logging level (e.g., INFO)
... or under its own prefix so that it can be nulled.
getLogger('msg.'+__name__) or however it winds up looking.
HM
Paul McLanahan
<freakb...@gmail.com> wrote:
> Secondly, I share Chris' concern about overloading 'messages'. This
> overlaps with the bikeshed issue of what to call the new contrib
> package - whatever name is picked for the contrib package should
> probably be reflected in the context variable, etc.
>
> Lastly, one issue that seems unaddressed is the transition plan for
> replacing user.message_set. The preamble to the wiki page makes a
> compelling argument for ditching message_set, but remains silent on a
> proposal for how Django (as a project) should be treating this new
> alternative.
>
> The solution might well be "use it if you want, ignore it if you
> don't", but given the problems with user.message_set, it might be
> worth giving some thought to a deprecation/transition plan.
This may be adding undue complexity, but I wanted to throw it out and
see what you thought anyway. I think a possible solution to Russ' last
two points may indeed be overloading the "messages" template variable.
What I'm thinking is that we keep the message_set (of course) and add
the new "request.messages.*" API, but completely remove the messages
part of the auth context_processor and have the default backend for
the new system pull both for the first release (purely for backward
comparability). Messages from the old message_set system could get a
default level in the new system. This shouldn't be an issue since the
people using it will likely be existing projects where they won't be
using any level information in their templates. We couple this
combined backend with settings that will allow you to easily disable
the DB part of the backend (or make it clear how to and strongly
suggest switching backends), and provide a setting for the default
message level that defaults to INFO.
I think this could ease transition as the two systems could live
together for the first release. The subsequent releases could switch
the default backend to the existing fallback one, and then eventually
remove the message_set part of the auth app all together.
I do recognize that this may create more confusion than is necessary.
It may also be an unacceptable performance drain on the new system to
support the old at all. I just think as someone who has projects that
still use the message_set, and especially considering the 3rd party
apps out there which may use it as well, it might be a nice balance to
be able to mix in the new and better while having as little impact on
the templates as possible.
Technically, this may be more of a challenge than it's worth however.
The current auth context_processor is in core, and it just really
feels wrong to me to couple this new app to either core or
contrib.auth. We could just remove the "messages" part of the auth
context_processor and add a new processor from the new app to the list
of defaults that will provide all of the "messages". And actually,
we'll need to at least override that part of the auth
context_processor anyway, or else we'll still get a db hit for every
request with a logged in user due to the call to
user.get_and_delete_messages.
Sorry about throwing my brainstorm at you. Perhaps it'll spark someone
else's solution.
Thanks,
Paul
Tobias McNulty
> proposed API. I'm not absolutely tied to this suggestion - I'm just
> noting that adding Django support for Python logging is also on the
> cards for v1.2, and it seems weird that we would introduce two APIs
> with similar external APIs, but not try to maintain parity.
I updated the API on the wiki. The levels/classes will likely be
different, because logging and messaging are two very different
things, but keeping the argument order the same is probably a good
thing.
> Secondly, I share Chris' concern about overloading 'messages'. This
> overlaps with the bikeshed issue of what to call the new contrib
> package - whatever name is picked for the contrib package should
> probably be reflected in the context variable, etc.
Okay, let's pick something other than 'messages' and stick to it. I'm
fine with "notifications", "notices", and maybe even "notes", but the
last one is a little ambiguous. Other ideas?
> Lastly, one issue that seems unaddressed is the transition plan for
> replacing user.message_set. The preamble to the wiki page makes a
> compelling argument for ditching message_set, but remains silent on a
> proposal for how Django (as a project) should be treating this new
> alternative.
>
> The solution might well be "use it if you want, ignore it if you
> don't", but given the problems with user.message_set, it might be
> worth giving some thought to a deprecation/transition plan.
I think the deprecation plan would look something like this:
1.2: user.message_set is officially deprecated but continues to work unaffected
1.3: user.message_set is still usable, but raises a deprecation
warning when it is used
1.4: user.message_set is removed
Are we comfortable ditching the auth Message model altogether (several
releases from now), or is someone actually using it for a legitimate
purpose that won't be supplanted by the proposed app?
Cheers,
Tobias
Russell Keith-Magee
>
> On Tue, Oct 13, 2009 at 3:27 AM, Russell Keith-Magee
> <freakb...@gmail.com> wrote:
>> Secondly, I share Chris' concern about overloading 'messages'. This
>> overlaps with the bikeshed issue of what to call the new contrib
>> package - whatever name is picked for the contrib package should
>> probably be reflected in the context variable, etc.
>
> Okay, let's pick something other than 'messages' and stick to it. I'm
> fine with "notifications", "notices", and maybe even "notes", but the
> last one is a little ambiguous. Other ideas?
I'll stay out of this particular bikeshed, other than to note that
Pinax ships with django-notifications, which has nothing to do with
session-based messages.
> I think the deprecation plan would look something like this:
>
> 1.2: user.message_set is officially deprecated but continues to work unaffected
> 1.3: user.message_set is still usable, but raises a deprecation
> warning when it is used
> 1.4: user.message_set is removed
I was hoping for a little more detail than this - this is just
Django's general deprecation plan, and I wouldn't expect that to
change. What exactly do these steps mean in practice? What changes are
going to be made to (for example) admin? Are there any other apps that
will be affected? What will happen when I update from 1.2-1.3?
1.3-1.4? Is there a need for a transition API (e.g., a 'message_set'
descriptor that actually sends messages to the new message backend)?
> Are we comfortable ditching the auth Message model altogether (several
> releases from now), or is someone actually using it for a legitimate
> purpose that won't be supplanted by the proposed app?
This is the sort of question we need to resolve for a deprecation
plan. Do we just delete auth.Message? Do we migrate it to an
contrib.old_messages app? I'm open to other suggestions.
Personally, I don't have a problem deprecating auth.Message, if for no
other reason than authentication has nothing to do with messaging.
Yours,
Russ Magee %-)
Tobias McNulty
> This is my main concern, and the thing keeping me from being
> enthusiastic about the current state of the proposal. Many apps --
> including, notably, django.contrib.admin -- rely on user.message_set.
> We can't simply remove it; we need to follow a deprecation schedule.
> But we also shouldn't just keep two parallel "messaging"
> implementations; that's silly.
>
> The best solution, I think, would be to implement user messages
> (user.message_set, get_and_delete_messages, etc) in terms of the new
> framework, and then gradually deprecate and remove the old APIs.
I think this is a large reason why the original attempt to get this
ticket fixed fell apart: User messages and session messages are two
completely different beasts and trying to fit them into the same API
is somewhat of a dead end.
The proposed app would function better for contrib.admin than what
exists now, so I'm confident that if we introduce/deprecate in a
gradual way the necessary changes can be made easily and on a
reasonable schedule. I don't envision updating the admin in this
release but someone correct me if there's an issue with not doing it
right away.
I'm not sure exactly what you mean by implement user messages in terms
of the new framework. It is completely possible to implement a
storage engine in several of the proposed solutions that would use
user.message_set to store data, though I'm not sure what the advantage
would be over any of the other options. Furthermore, the fact that
user.message_set is just a related manager on a model and not an
independent API per se makes it a difficult one to remove while
simultaneously keeping user message functionality.
Unless there is a compelling use case for user-based messages that
really sets it apart from the proposed app (and there may very well
be, I just haven't heard it yet), my vote is to deprecate the Message
model in contrib.auth in a gradual way and remove it altogether
several releases down the road.
Tobias
Tobias McNulty
> This may be adding undue complexity, but I wanted to throw it out and
> see what you thought anyway. I think a possible solution to Russ' last
> two points may indeed be overloading the "messages" template variable.
> What I'm thinking is that we keep the message_set (of course) and add
> the new "request.messages.*" API, but completely remove the messages
> part of the auth context_processor and have the default backend for
> the new system pull both for the first release (purely for backward
> comparability). Messages from the old message_set system could get a
> default level in the new system. This shouldn't be an issue since the
> people using it will likely be existing projects where they won't be
> using any level information in their templates. We couple this
> combined backend with settings that will allow you to easily disable
> the DB part of the backend (or make it clear how to and strongly
> suggest switching backends), and provide a setting for the default
> message level that defaults to INFO.
As a developer I'd rather make the conscious switch from one
technology to the other. IMHO I don't think the effort and potential
confusion of making everyone's templates "just work" is worth it.
Chris, I know you have some personal experience trying to get session-
and user-based messages to work together, so I think we'd benefit from
any wisdom you might be able to offer. :)
Toby
Tobias McNulty
1.2 feature votes:
* The proposed messaging app will definitely leverage the cookie
signing utilities also in store for 1.2.
* This is a compelling core feature for me, at least, because a
standard needs to be set that reusable apps can take advantage of.
Currently everyone rolls his or her own solution or depends on (with
all due respect) some random 3rd party app (or at least that's how it
feels when one is trying to integrate two apps that use different
messaging backends).
* Developing a plan to integrate with user.message_set (and
potentially deprecate the old API) is a key priority and something we
DO need to hammer out more. That said I think we can get it done.
Tobias
Tobias McNulty
<freakb...@gmail.com> wrote:
> I was hoping for a little more detail than this - this is just
> Django's general deprecation plan, and I wouldn't expect that to
> change. What exactly do these steps mean in practice? What changes are
> going to be made to (for example) admin? Are there any other apps that
> will be affected? What will happen when I update from 1.2-1.3?
> 1.3-1.4? Is there a need for a transition API (e.g., a 'message_set'
> descriptor that actually sends messages to the new message backend)?
I put up a skeleton of a plan (not much more than what I sent before)
on the wiki:
http://code.djangoproject.com/wiki/SessionMessages#DeprecationPlan
There are still a few things missing (like what apps actually need to
be updated) but hopefully that'll provide us a place to keep track of
the details.
Paul McLanahan
> The best solution, I think, would be to implement user messages
> (user.message_set, get_and_delete_messages, etc) in terms of the new
> framework, and then gradually deprecate and remove the old APIs.
On Tue, Oct 13, 2009 at 10:30 PM, Tobias McNulty <tob...@caktusgroup.com> wrote:
> I'm not sure exactly what you mean by implement user messages in terms
> of the new framework. It is completely possible to implement a
> storage engine in several of the proposed solutions that would use
> user.message_set to store data, though I'm not sure what the advantage
> would be over any of the other options. Furthermore, the fact that
> user.message_set is just a related manager on a model and not an
> independent API per se makes it a difficult one to remove while
> simultaneously keeping user message functionality.
I took what Jacob said to be the opposite. It sounded to me like he
was suggesting that we keep the existing user messaging api, but
change the internals of it to use the new notifications framework for
storage. In my head it goes something like:
1. Remove the relation from the User to the Message model.
2. Add a message_set property that mimics the related manager, but
which stores the messages via the new notifications app.
3. Modify the get_and_delete_messages method to retrieve messages from
the new app.
4. Gradually remove the messaging stuff from contrib.auth all together.
This has the advantage of removing the need to modify
core.context_managers.auth since that just uses
User.get_and_delete_messages.
I think this method is nice and clean, backward compatible as far as
the API goes, and moves all messaging into a single backend w/o
disrupting existing view or template code. It may break things if
people are directly querying the Message model, but that has to be
extremely rare.
Paul
Jacob Kaplan-Moss
> I took what Jacob said to be the opposite. It sounded to me like he
> was suggesting that we keep the existing user messaging api, but
> change the internals of it to use the new notifications framework for
> storage. In my head it goes something like:
Right, that's exactly what I meant; thanks.
I'm trying to avoid having two incompatible messaging systems in
Django. I agree that linking messages to sessions makes more sense
than linking them to users [1], but we can't just remove the user
messages API outright. I'd like a good transition strategy, so it
seems the best way would be to keep the user messages API, but switch
it to use session messages under the hood.
Or am I missing something?
Jacob
[1] Just as a historical note: the message API was never really
intended for public consumption; it was added as a lightweight way of
giving success messages in the admin. It was originally really just a
hardwired in part of the admin, and as such I'm not surprised it
doesn't work for a lot of other uses.
Tai Lee
>
> Okay, let's pick something other than 'messages' and stick to it. I'm
> fine with "notifications", "notices", and maybe even "notes", but the
> last one is a little ambiguous. Other ideas?
nothing to add that hasn't already been mentioned. I've been using
django-notify for a while now, and think it's great. Can't wait to see
something like it make it's way into core.
Cheers.
Tai.
Tobias McNulty
> My personal bike shed is named "alerts", but other than that I have
> nothing to add that hasn't already been mentioned. I've been using
> django-notify for a while now, and think it's great. Can't wait to see
> something like it make it's way into core.
That's an option. It's short and doesn't conflict with anything that
I know of. I added a section to the wiki with potential app names:
http://code.djangoproject.com/wiki/SessionMessages#PotentialAppNames
Tobias McNulty
> I'm trying to avoid having two incompatible messaging systems in
> Django. I agree that linking messages to sessions makes more sense
> than linking them to users [1], but we can't just remove the user
> messages API outright. I'd like a good transition strategy, so it
> seems the best way would be to keep the user messages API, but switch
> it to use session messages under the hood.
I know a solution like this was attempted and then abandoned earlier
in the ticket history.
Chris, do you have any wisdom to share about this? I expect it may be
complicated because I can't think of a clean way to get the current
request (and hence the session messages variable) inside the User
model.
Maybe I'm missing something.
SmileyChris
On Oct 16, 5:56 am, Tobias McNulty <tob...@caktusgroup.com> wrote:
> On Wed, Oct 14, 2009 at 10:27 AM, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:
> > I'm trying to avoid having two incompatible messaging systems in
> > Django. I agree that linking messages to sessions makes more sense
> > than linking them to users [1], but we can't just remove the user
> > messages API outright. I'd like a good transition strategy, so it
> > seems the best way would be to keep the user messages API, but switch
> > it to use session messages under the hood.
>
> I know a solution like this was attempted and then abandoned earlier
> in the ticket history.
* user messages were left as they are (database call and all)
* session messages were added
* a new context processor handled lazily combining both message
sources into a {{ messages }} context variable
>
> Chris, do you have any wisdom to share about this? I expect it may be
> complicated because I can't think of a clean way to get the current
> request (and hence the session messages variable) inside the User
> model.
ing).
If we are to slowly deprecate this functionality, I think that we
should do effectively the same thing.
Leave user messages as-is and use something like this as the default
message storage:
class LegacyFallbackStorage(FallbackStorage):
storage_classes = (UserMessageStorage, CookieStorage,
SessionStorage)
If someone knows they don't need user messages at all, they can set
NOTIFICATIONS_STORAGE = FallbackStorage
SmileyChris
>
> class LegacyFallbackStorage(FallbackStorage):
> storage_classes = (UserMessageStorage, CookieStorage,
> SessionStorage)
No need to panic, django-notify users - I haven't made it the default
storage.
Tobias McNulty
> If we are to slowly deprecate this functionality, I think that we
> should do effectively the same thing.
> Leave user messages as-is and use something like this as the default
> message storage:
Just to make sure I understand this correctly, let me try to summarize
what this would mean:
* the proposed app controls {{ messages }} and is responsible for
retrieving anything set in the Message model
* old apps that call user.message_set.create will still get their
messages to the screen, assuming the project uses {{ messages }} in
the template
* old apps that call get_and_delete_messages or iterate through the
Message.objects.all() manually will NOT see any messages that were
stored in the session or a cookie, but they will continue to see
messages created the old way
* judging from your code, the new app will NOT store any messages in
the old Message model, it will only read them
* the standard template code:
{% if messages %}
<ul>
{% for message in messages %}
<li>{{ message }}</li>
{% endfor %}
</ul>
{% endif %}
will continue to work untouched after the update. For those that
choose to leverage it, a {{message.level}} (or otherwise named
attribute) will also be available.
* since we are tying ourselves to the {{ messages }} variable, this
probably means that the app should be called 'messages'. There has
been some hesitation about this. Are we in the clear now that we have
a potential upgrade path from the old API? Or is there still concern
about naming?
SmileyChris
> Just to make sure I understand this correctly, let me try to summarize
> what this would mean:
>
> * the proposed app controls {{ messages }} and is responsible for
> retrieving anything set in the Message model
contrib.auth processor continue to set {{ messages }} - we'll just
override it (and state in docs that it must be placed after
contrib.auth processor)
>
> * old apps that call user.message_set.create will still get their
> messages to the screen, assuming the project uses {{ messages }} in
> the template
>
> * old apps that call get_and_delete_messages or iterate through the
> Message.objects.all() manually will NOT see any messages that were
> stored in the session or a cookie, but they will continue to see
> messages created the old way
>
> * judging from your code, the new app will NOT store any messages in
> the old Message model, it will only read them
>
> * the standard template code:
> choose to leverage it, a {{message.level}} (or otherwise named
> attribute) will also be available.
> * since we are tying ourselves to the {{ messages }} variable, this
> probably means that the app should be called 'messages'. There has
> been some hesitation about this. Are we in the clear now that we have
> a potential upgrade path from the old API? Or is there still concern
> about naming?
indicated that the User messages should be deprecated.
It makes the upgrade path easier.
Luke Plant
I've been catching up on this and watching this evolve, and this
proposal now looks solid IMO.
I think you should call this 'messages' consistently throughout, in
terms of the names of modules/variables/settings. If you need to
changed some documentation regarding the existing message
functionality, call that 'user messages' to distinguish it.
With regard to the deprecation schedule, normally we add a
PendingDeprecationWarning before the DeprecationWarning. If code in
core/contrib continues to use user messages for 1.2, it will emit a
PendingDeprecation, which is bad practice really - it's not fair to
users to emit warnings that aren't their fault and that they can't
fix, and it's not good for core code to be using something that we are
saying everyone else shouldn't.
I think this means that either the deprecation cycle would have to
pushed back one (i.e. pending deprecation warning in 1.3, deprecation
in 1.4, removed in 1.5), or core/contrib should be fixed by 1.2. I
would strongly prefer the latter, and this would affect my vote: I
don't want two messaging systems in Django, and if user messages are
not deprecated, then we do have two systems.
Luke
--
You meet a lot of smart guys with stupid wives, but you almost
never meet a smart woman with a stupid husband. (Erica Jong)
Luke Plant || http://lukeplant.me.uk/
Tobias McNulty
> I think this means that either the deprecation cycle would have to
> pushed back one (i.e. pending deprecation warning in 1.3, deprecation
> in 1.4, removed in 1.5), or core/contrib should be fixed by 1.2. I
> would strongly prefer the latter, and this would affect my vote: I
> don't want two messaging systems in Django, and if user messages are
> not deprecated, then we do have two systems.
I agree that using deprecated code in the core is setting a bad
example. A quick review of the code shows 8 calls to
message_set.create: 3 in auth, 2 in admin, and 3 in the
create/update/delete family of generic views. This definitely sounds
to me like a manageable update for 1.2.
Per feedback from Jacob, Chris, and Luke, I updated the notes on the
existing API, the transition plan, and the potential API on the wiki:
http://code.djangoproject.com/wiki/SessionMessages
Cheers,
Tobias
David Cramer
----
David Cramer
Tobias McNulty
a contrib.messages app heavily based on django_notify (only
renamed/updated to reflect the API on the wiki):
http://bitbucket.org/tobias.mcnulty/django-contrib-messages/
It's a work in progress and I'll be updating it in the coming
days/weeks but feel free to try it out/push back any changes you see
fit.
Tobias
Tobias McNulty
> should be done with the inclusion of the messages patch.
I'm working on migrating auth, admin, and the create_update generic
views to use messages in my bitbucket branch.
I updated them all to use request.messages.add() and things work great
as long as one has the middleware and context processor enabled for
django.contrib.messages. This obviously won't be the case, however,
when everyone initially updates to Django 1.2. There are a couple
ways around this that I see:
1) raise an error stating that the MessageMiddleware and 'messages'
context processor need to be installed, like the admin does for the
SessionMiddleware, e.g.:
assert hasattr(request, 'messages'), "The Django admin
requires message middleware to be installed. Edit your
MIDDLEWARE_CLASSES setting to insert
'django.contrib.messages.middleware.MessageMiddleware'."
2) add a compatibility method to the messages app that looks for
messages and falls back to user.message_set if the middleware hasn't
been enabled, e.g.:
def compat_add_message(request, level, message):
"""
Attempt to add a message to the request using the 'messages' app, falling
back to the user's message_set if MessageMiddleware hasn't been enabled.
"""
if hasattr(request, 'messages'):
request.messages.add(level, message)
elif hasattr(request, 'user') and request.user.is_authenticated():
request.user.message_set.create(message=message)
In sticking with the staged deprecation policy, my vote would be:
* use compat_add_message() internally in 1.2. There is no need to
document this function as request.message_set.create will continue to
work.
* in 1.3, remove compat_add_message and raise an error if 'messages'
isn't enabled when you try to use the admin or create_update generic
views, etc.
I could be convinced to skip the compatibility function and go
straight to raising an error, but that seems like a big jump.
I updated the branch to do basically this, but nothing is set in stone.
Thoughts?
Luke Plant
> In sticking with the staged deprecation policy, my vote would be:
> * use compat_add_message() internally in 1.2. There is no need to
> document this function as request.message_set.create will continue
> to work.
> * in 1.3, remove compat_add_message and raise an error if
> 'messages' isn't enabled when you try to use the admin or
> create_update generic views, etc.
I think this is the right approach. The second branch of
compat_add_message should raise a DeprecationWarning that indicates
that the 'messages' app will be a dependency of the admin and generic
views, since the user's code is going to break if they update to 1.3
without fixing their settings. Obviously the release notes for 1.2
and 1.3 need to detail this - you can start to add things to
doc/releases/1.2-alpha.txt
I've started to look at the code, and have some other minor
suggestions, but not significant things to do with approach, so I'll
e-mail you personally with them at some point. On the basis of what
I've seen so far, I'm willing to change my vote on this to +1, which
at the moment would make me the 'champion' for this feature, as no-one
has voted -1. At the moment, the big areas that are lacking are:
- user docs
- code docs (I'm curious about things like EOFMessage, there
is nothing to explain why it exists).
- changes and complete tests for the auth context processor,
including the different configurations that are allowed
(with and without the messages app, for instance). I think
the auth context processor should call the messages one,
so that you get the same result whether you use one or
the other or both.
Luke
--
Carpe post meridian! (I'm not a morning person.)
Luke Plant || http://lukeplant.me.uk/
Tobias McNulty
> I've started to look at the code, and have some other minor
> suggestions, but not significant things to do with approach, so I'll
> e-mail you personally with them at some point. On the basis of what
> I've seen so far, I'm willing to change my vote on this to +1, which
> at the moment would make me the 'champion' for this feature, as no-one
> has voted -1. At the moment, the big areas that are lacking are:
>
> - user docs
> - code docs (I'm curious about things like EOFMessage, there
> is nothing to explain why it exists).
> - changes and complete tests for the auth context processor,
> including the different configurations that are allowed
> (with and without the messages app, for instance). I think
> the auth context processor should call the messages one,
> so that you get the same result whether you use one or
> the other or both.
Thanks for your support and feedback. I'll get started on the docs
and other changes you mention. There's a small issue with the auth
context processor returning what the messages one does: it's currently
the responsibility of the middleware to create the Storage object that
is request.messages, so this won't exist if the middleware isn't
installed.
We could: (a) instantiate a new Storage instance for the {{ messages
}} variable if request.messages doesn't exist (in the context
processor), or (b) introduce another compat_* method that tries to
retrieve messages from request.messages, and falls back to
user.message_set if the middleware hasn't been enabled. I'd be more
comfortable with (b) since I'm not certain what all the ramifications
of (a) would be (and there wouldn't be much point anyway, since
there'd be no way to get a message into the cookie or session storages
without the middleware).
Now that I think about it, this is probably what you meant and just
wanted to make sure the messages context_processor will do the same
thing. I'll update the code.
Tobias McNulty
http://bitbucket.org/tobias.mcnulty/django-contrib-messages/src/tip/docs/ref/contrib/messages.txt
If your comments wouldn't benefit from discussion, feel free to email them to my privately to avoid swamping the list.
Thanks,
Waylan Limberg
> I've added some user docs to my bitbucket branch and would much appreciate
> any feedback from the group,
I couldn't help but notice the docs suggest setting MESSAGE_STORAGE like so:
MESSAGE_STORAGE = 'cookie.CookieStorage'
Just yesterday, in reference to another upcoming feature, Jacob
commented that he would prefer all such settings to require full paths
only. In other words you would remove the attempt to import from a
default location first. So the correct setting would be:
MESSAGE_STORAGE = 'django.contrib.messages.storage.cookie.CookieStorage'
Obviously, it's not my place to say for sure, but I got the impression
this should apply to all new features.
--
----
\X/ /-\ `/ |_ /-\ |\|
Waylan Limberg
Jacob Kaplan-Moss
> I couldn't help but notice the docs suggest setting MESSAGE_STORAGE like so:
>
> MESSAGE_STORAGE = 'cookie.CookieStorage'
>
> Just yesterday, in reference to another upcoming feature, Jacob
> commented that he would prefer all such settings to require full paths
> only. In other words you would remove the attempt to import from a
> default location first. So the correct setting would be:
>
> MESSAGE_STORAGE = 'django.contrib.messages.storage.cookie.CookieStorage'
>
> Obviously, it's not my place to say for sure, but I got the impression
> this should apply to all new features.
Yes, please.
Special-casing built-in backends requires more code and leads to the
impression that external backends are somehow second-class citizens
This isn't -- or shouldn't be -- true.
Jacob
Tobias McNulty
Yes, please.
Special-casing built-in backends requires more code and leads to the
impression that external backends are somehow second-class citizens
This isn't -- or shouldn't be -- true.
Russell Keith-Magee
> I've added some user docs to my bitbucket branch and would much appreciate
> any feedback from the group, as I'm much too steeped in it at this point to
> tell what's clear and what isn't:
>
> http://bitbucket.org/tobias.mcnulty/django-contrib-messages/src/tip/docs/ref/contrib/messages.txt
>
> If your comments wouldn't benefit from discussion, feel free to email them
> to my privately to avoid swamping the list.
Hi Tobias,
These docs look like a pretty good start to me. Here's some feedback:
* In the discussion about message levels mentions tags before tags
have been introduced. The section on levels should introduce the
messaging levels without reference to the tagging process.
* The explanation of tags says they are a 'string representation',
but doesn't clarify what that string is useful for. Is it a verbose,
human readable name? An identifier? A name suitable for a CSS class?
* There also seems to be a lot of attention paid to the numerical
values for constants. Ordinarily, you don't really care about the
numerical values of system constants - you just use them. To my mind,
the numerical values of the constants are irrelevant until you get to
the 'custom level' section - where they are only relevant because you
need to know the numbers you can't use.
* The example custom level CRITICAL doesn't have a consistent value -
it's 50 in places, 60 in others.
* The explanation around minimum recorded message levels could do
with some elaboration. It isn't entirely clear to me what the
message.level setting does - is it a minimum level? maximum level?
inclusive or exclusive? The example here needs to be richer - showing
an example of a message that _won't_ be stored, for example.
* Similarly for extra tags - are the extra tags concatenated onto
message.tags? Are spaces added? will message.tags become a list?
* The description of LegacyFallbackStorage storage could do with some
elaboration. I'd almost go so far as to say that there should be a
whole section in the docs about this storage backend, and exactly how
it operates - the original list where the storage engines are
introduced should provide a link to a more detailed section. Questions
that need to be addressed:
* In what order are messages retrieved from cookie,session and database?
* Do I need to do anything to my existing user.message_set code?
* What level do old-style messages get?
* Following the lead of contrib.comments, you may want to consider
splitting the settings documentation into a separate documentation
module.
On a code level - I noticed that the messages middleware is in the
list of defaults in the settings.py template, but not in
global_settings.py. A minor inconsistency, but one I noticed while
trying to work out exactly what the transition requirements would be
for existing projects.
I had a quick poke around the rest of the code base, especially about
transition issues. However, I didn't want to start in on a big
teardown unless you thought the code was ready for it. When you're
ready for a the full latex-glove treatment, let me know. :-)
Yours,
Russ Magee %-)
Tobias McNulty
* In the discussion about message levels mentions tags before tags
have been introduced. The section on levels should introduce the
messaging levels without reference to the tagging process.
* The explanation of tags says they are a 'string representation',
but doesn't clarify what that string is useful for. Is it a verbose,
human readable name? An identifier? A name suitable for a CSS class?
* There also seems to be a lot of attention paid to the numerical
values for constants. Ordinarily, you don't really care about the
numerical values of system constants - you just use them. To my mind,
the numerical values of the constants are irrelevant until you get to
the 'custom level' section - where they are only relevant because you
need to know the numbers you can't use.
* The example custom level CRITICAL doesn't have a consistent value -
it's 50 in places, 60 in others.
* The explanation around minimum recorded message levels could do
with some elaboration. It isn't entirely clear to me what the
message.level setting does - is it a minimum level? maximum level?
inclusive or exclusive? The example here needs to be richer - showing
an example of a message that _won't_ be stored, for example.
* Similarly for extra tags - are the extra tags concatenated onto
message.tags? Are spaces added? will message.tags become a list?
I think it basically said that already but I tried to clarify what was there, and added some explanation about how tags are stored, in general, in the first section on tags.
I'd be open to the argument that tags should be stored in a list instead of a string, in case you want to do other things with them and/or space delineation doesn't work for you.
* The description of LegacyFallbackStorage storage could do with some
elaboration. I'd almost go so far as to say that there should be a
whole section in the docs about this storage backend, and exactly how
it operates - the original list where the storage engines are
introduced should provide a link to a more detailed section. Questions
that need to be addressed:
* In what order are messages retrieved from cookie,session and database?
* Do I need to do anything to my existing user.message_set code?
* What level do old-style messages get?
Added.
* Following the lead of contrib.comments, you may want to consider
splitting the settings documentation into a separate documentation
module.
Seems silly for just 3 settings, especially since the settings are already documented on a separate page (the full list of settings), but it looks like that's all comments has too. So, if it's the new convention and other apps are doing the same, I'll split it up.
On a code level - I noticed that the messages middleware is in the
list of defaults in the settings.py template, but not in
global_settings.py. A minor inconsistency, but one I noticed while
trying to work out exactly what the transition requirements would be
for existing projects.
Whoops, thanks for catching that. Added.
I had a quick poke around the rest of the code base, especially about
transition issues. However, I didn't want to start in on a big
teardown unless you thought the code was ready for it. When you're
ready for a the full latex-glove treatment, let me know. :-)
Will do. I have a little cleanup to do regarding EOFMessage per some feedback from Luke Plant but I think it should be ready soon.
Thanks for all your feedback thus far!
Tobias
Tobias McNulty
I had a quick poke around the rest of the code base, especially about
transition issues. However, I didn't want to start in on a big
teardown unless you thought the code was ready for it. When you're
ready for a the full latex-glove treatment, let me know. :-)
Tear it apart :-)
Diff showing latest changes in branch uploaded to ticket:
http://code.djangoproject.com/attachment/ticket/4604/django-contrib-messages-9f54c0f8719c.diff
Cheers,
Russell Keith-Magee
> On Thu, Nov 5, 2009 at 9:15 AM, Russell Keith-Magee <freakb...@gmail.com>
> wrote:
>>
>> I had a quick poke around the rest of the code base, especially about
>> transition issues. However, I didn't want to start in on a big
>> teardown unless you thought the code was ready for it. When you're
>> ready for a the full latex-glove treatment, let me know. :-)
>
> Tear it apart :-)
or cosmetic:
* importcls: I can see that the aim here is to reduce code
duplication. However, I'm not convinced it's worth it - the code
you're replacing isn't that complex to start with, only occurs in two
places, and the price paid is a loss of fidelity in the error message.
On top of that, it doesn't satisfy the use cases for the other
pluggable backends (like cache and mail). In this case, I'm happy to
live with the code duplication rather than introduce a new utility
module.
* contrib.messages.compat: I'm not a huge fan of the deprecation
approach here. What you've got in code here requires that when v1.4
comes around, we don't just need to delete deprecated methods, we (and
anyone else writing pluggable apps that need to support messages) need
to do code updates to remove usage of deprecated methods.
The compat_add_message() and compat_get_messages() calls are
essentially just shortcuts - so why not formalize it as the official
API. i.e.,:
from django.contrib import messages
messages.add_message(request, level, msg)
When v1.4 comes around, we will need to tweak the implementation of
add_message() to remove the deprecated code path, but that's the
extent of the code changes required by us or any third party using
messages.
This has the additional benefit of separating the implementation from
the API by a degree - if we ever choose to replace request.messages
with something that isn't bound to the request itself, the
message.add_message() api can remain.
* I think there's a circular reference problem in the middleware with
the setup for request.messages. The messages middleware actually
creates request.messages, but the auth middleware creates a lazy
evaluator that can return request.messages. This means that as a
request comes in, the request is always given a lazy evaluator for
user.messages_set, which is then overwritten by the messages
middleware. The consequence of this is that any middleware that is
executed between the auth and messages processors will get
user.message_set if they try to set a message.
* Message._get_tags() can be simplified by using ' '.join([extra,label])
* Is there a use case for backends to use store_serialized=False?
There doesn't appear to be one in the shipped backends.
* Is there a use case for the 'fail_silently' flag on update? it
doesn't appear to be used.
* I might be missing something subtle, but it appears that the
update() method is calling _prepare() on messages that have already
been prepared - namely, the _loaded_messages in the added_new case.
Shouldn't loaded messages already by prepared?
* In the cookie backend, the import of django.utils.simplejson will
check for a locally available version of json - there's no need for
the import check.
* Also in the cookie backend - Is it just me, or is
MessageDecoder.process_messages much more complex than is required? It
seems to be able to handle decoding lists of lists of messages and
dicts of messages - but I'm not sure I understand why. You can delete
lines 38-41, and the tests still pass.
* The _store() method on the session backend appears to rely on
implicit return values.
* Shouldn't the LegacyFallbackStorage class be using
user._message_set, rather than the message_set wrapper? If the user is
using a known legacy wrapper, we don't need to raise
PendingDeprecation warnings every time they use the backend.
* I have some concerns about the thread-safety (or, rather,
request-safety) of the various backends. If I'm reading the code
right, if you have two parallel requests, a message written in one
response will overwrite a message written in the second, which will
result in the first message never being displayed to the user. This
wasn't a problem with the old API because messages were written to the
database on response, and read on request; the first request would get
all the messages. If we're using cookies and sessions, there's all
sorts of clobbering problems.
* The 1.2 release docs will need an entry describing this changeset,
both for the feature and the upgrade steps required.
* The deprecation notes should explicitly mention all the APIs that
will be deprecated - the current text of "and associated APIs" isn't
particularly specific about what will be deprecated.
* The updated documentation about the auth context processor
shouldn't mention the old-style user.message_set API. If it's
deprecated, it's deprecated. However, it's ok to mention the old API
in the versionchanged note. (i.e., Prior to v1.2, request.messages was
a lazy accessor for user.message_set)
* In the auth docs, you can use the ..deprecated sphinx tag as the
analog for ..versionadded and ..versionchanged.
* In the docs about LegacyFallbackStorage, you don't need to repeat
or explain Django's deprecation policy - just say that the policy will
be followed and provide a link to the original source.
... and I'm spent. On the whole, this is looking pretty good. The
cosmetic problems won't take much to fix. The only two problems that
are concerning to me are the circular reference problem and the
potential for threading/parallel request problems. In both cases, the
reason I'm concerned is that the solution isn't immediately obvious.
That doesn't mean that the solution will necessarily be difficult -
just that it will require some consideration.
Yours,
Russ Magee %-)
Tobias McNulty
* I think there's a circular reference problem in the middleware with
the setup for request.messages. The messages middleware actually
creates request.messages, but the auth middleware creates a lazy
evaluator that can return request.messages. This means that as a
request comes in, the request is always given a lazy evaluator for
user.messages_set, which is then overwritten by the messages
middleware. The consequence of this is that any middleware that is
executed between the auth and messages processors will get
user.message_set if they try to set a message.
The auth middleware now returns the same thing the messages middleware does, so that may change the problem a little. Still, I'm not sure what the consequence would be of using messages between the two middlewares. I'll give it some thought and propose/implement a fix if it looks problematic.
* I have some concerns about the thread-safety (or, rather,
request-safety) of the various backends. If I'm reading the code
right, if you have two parallel requests, a message written in one
response will overwrite a message written in the second, which will
result in the first message never being displayed to the user. This
wasn't a problem with the old API because messages were written to the
database on response, and read on request; the first request would get
all the messages. If we're using cookies and sessions, there's all
sorts of clobbering problems.
True. One simple solution would be to offer another "thread safe" backend that used the database as a storage (independent of the User model of course).
As long as we note the lack of thread safety in the cookie and session backends I think they'll still be alright for most cases; hopefully this is something that folks using sessions and cookies are prepared to deal with.
Tobias
Russell Keith-Magee
> Hi Russell, thanks for all your feedback. I'll review the small issues in a
> separate email (or just fix them), but here are my responses to the two
> bigger problems.
>
> On Fri, Nov 20, 2009 at 3:29 AM, Russell Keith-Magee
> <freakb...@gmail.com> wrote:
>>
>> * I think there's a circular reference problem in the middleware with
>> the setup for request.messages. The messages middleware actually
>> creates request.messages, but the auth middleware creates a lazy
>> evaluator that can return request.messages. This means that as a
>> request comes in, the request is always given a lazy evaluator for
>> user.messages_set, which is then overwritten by the messages
>> middleware. The consequence of this is that any middleware that is
>> executed between the auth and messages processors will get
>> user.message_set if they try to set a message.
>
> The auth middleware now returns the same thing the messages middleware does,
> so that may change the problem a little. Still, I'm not sure what the
> consequence would be of using messages between the two middlewares. I'll
> give it some thought and propose/implement a fix if it looks problematic.
brainfart on my part. I was looking at the context processor code for
auth and mentally mapping it to the auth middleware for some reason.
However, there is still a lesser version of the same problem -
request.messages.add() will fail outright if you try to invoke it
before the messages middleware has executed, but compat_add_message()
function won't fail in the same circumstances.
>> * I have some concerns about the thread-safety (or, rather,
>> request-safety) of the various backends. If I'm reading the code
>> right, if you have two parallel requests, a message written in one
>> response will overwrite a message written in the second, which will
>> result in the first message never being displayed to the user. This
>> wasn't a problem with the old API because messages were written to the
>> database on response, and read on request; the first request would get
>> all the messages. If we're using cookies and sessions, there's all
>> sorts of clobbering problems.
>
> True. One simple solution would be to offer another "thread safe" backend
> that used the database as a storage (independent of the User model of
> course).
>
> As long as we note the lack of thread safety in the cookie and session
> backends I think they'll still be alright for most cases; hopefully this is
> something that folks using sessions and cookies are prepared to deal with.
experienced with sessions and cookies, I can imagine it being
extremely surprising to anybody looking at contrib.messages as a
"messaging system" independent of internals.
A big notice that says "messages aren't guaranteed to be delivered, or
guaranteed to be delivered into the window that created them" is the
very least we can do here. It would be nice if we could do more, but
I'm not sure there is a practical solution (other than providing a
database-backed store).
Yours,
Russ Magee %-)
Tobias McNulty
Thanks for all your feedback. I think I've got everything covered except for a few questions about the API (see below).
* importcls: I can see that the aim here is to reduce code
duplication. However, I'm not convinced it's worth it - the code
you're replacing isn't that complex to start with, only occurs in two
places, and the price paid is a loss of fidelity in the error message.
On top of that, it doesn't satisfy the use cases for the other
pluggable backends (like cache and mail). In this case, I'm happy to
live with the code duplication rather than introduce a new utility
module.
Okay, I reverted it for now. It could always be abstracted later if it seems worth it. I haven't tracked down all the other potential uses to see if a generic implementation would work, but I think there are a few beyond cache and mail.
Luke originally suggested that it be abstracted, so I'll let him argue the point if he chooses. :)
* contrib.messages.compat: I'm not a huge fan of the deprecation
approach here. What you've got in code here requires that when v1.4
comes around, we don't just need to delete deprecated methods, we (and
anyone else writing pluggable apps that need to support messages) need
to do code updates to remove usage of deprecated methods.
The compat_add_message() and compat_get_messages() calls are
essentially just shortcuts - so why not formalize it as the official
API. i.e.,:
messages.add_message(request, level, msg)
from django.contrib import messages
When v1.4 comes around, we will need to tweak the implementation of
add_message() to remove the deprecated code path, but that's the
extent of the code changes required by us or any third party using
messages.
This has the additional benefit of separating the implementation from
the API by a degree - if we ever choose to replace request.messages
with something that isn't bound to the request itself, the
message.add_message() api can remain.
I like that. The compat_* family of methods were not intended for use outside of Django itself (some comments at the top of compat.py should say as much). Still, I can foresee a few situations in which you'd need to use them (or implement your own) if you're trying to build a project that uses both old- and new-style messaging. So, I think implementing an API that's decoupled from request might be a good thing.
How would your approach work for the shortcut methods, like info, debug, etc.?
I'm not sure I like the possibility that one could:
from django.contrib.messages import debug
debug('a message')
As it may need to a lot of import renaming for the sake of not polluting the namespace with such generic names--and I don't want to force people to:
from django.contrib import messages
As not everyone may like having 'messages' in their namespace either.
Maybe we need a getLogger()-like method? What about something like:
from django.contrib.messages import get_message_store
msg_store = get_message_store(request)
msg_store.info('a message')
I don't really like my choice of variable/method names, but you get the idea. Better naming ideas welcome. :)
* Message._get_tags() can be simplified by using ' '.join([extra,label])
Fixed.
* Is there a use case for backends to use store_serialized=False?
There doesn't appear to be one in the shipped backends.
I couldn't think of a reason that it would hurt to force_unicode on the message and level before storing, so I removed it. If there's something we missed, let us know, Chris. :)
* Is there a use case for the 'fail_silently' flag on update? it
doesn't appear to be used.
Removed.
* I might be missing something subtle, but it appears that the
update() method is calling _prepare() on messages that have already
been prepared - namely, the _loaded_messages in the added_new case.
Shouldn't loaded messages already by prepared?
Fixed.
* In the cookie backend, the import of django.utils.simplejson will
check for a locally available version of json - there's no need for
the import check.
Removed, thanks.
* Also in the cookie backend - Is it just me, or is
MessageDecoder.process_messages much more complex than is required? It
seems to be able to handle decoding lists of lists of messages and
dicts of messages - but I'm not sure I understand why. You can delete
lines 38-41, and the tests still pass.
Tests added that will break if you remove those lines now. :)
The list traversal is entirely necessary as that's what the encoder is given - a list of messages. I added the dictionary traversal to make the implementation more complete, though it's not really needed for the way messages are currently stored.
* The _store() method on the session backend appears to rely on
implicit return values.
Fixed.
* Shouldn't the LegacyFallbackStorage class be using
user._message_set, rather than the message_set wrapper? If the user is
using a known legacy wrapper, we don't need to raise
PendingDeprecation warnings every time they use the backend.
Fixed.
* The 1.2 release docs will need an entry describing this changeset,
both for the feature and the upgrade steps required.
Added.
* The deprecation notes should explicitly mention all the APIs that
will be deprecated - the current text of "and associated APIs" isn't
particularly specific about what will be deprecated.
Fixed.
* The updated documentation about the auth context processor
shouldn't mention the old-style user.message_set API. If it's
deprecated, it's deprecated. However, it's ok to mention the old API
in the versionchanged note. (i.e., Prior to v1.2, request.messages was
a lazy accessor for user.message_set)
Fixed.
* In the auth docs, you can use the ..deprecated sphinx tag as the
analog for ..versionadded and ..versionchanged.
Added.
* In the docs about LegacyFallbackStorage, you don't need to repeat
or explain Django's deprecation policy - just say that the policy will
be followed and provide a link to the original source.
Removed.
Additionally, per your other email, I added some notes to the docs about behavior of parallel requests from the same client.
Again, the only thing I can think of that still needs to be worked out is the details of the API and whether or not it should be decoupled from request. I'm in favor of the idea, I'm just not sure what solution makes the best sense yet (see above).
Russell Keith-Magee
from django.contrib import messages as i_dont_want_to_have_name_clashes
:-)
Seriously, this is exactly what 'import as' is designed for. If the
end user is worried about generic names, then both:
from django.contrib import messages as msg
msg.debug('foo')
and
from django.contrib.messages import debug as debug_msg
are viable workarounds.
> Maybe we need a getLogger()-like method? What about something like:
>
> from django.contrib.messages import get_message_store
> msg_store = get_message_store(request)
> msg_store.info('a message')
>
> I don't really like my choice of variable/method names, but you get the
> idea. Better naming ideas welcome. :)
it. You're turing a simple method call into two, all to avoid a
possible naming collision that Python gives you the tools to avoid
anyway.
> Again, the only thing I can think of that still needs to be worked out is
> the details of the API and whether or not it should be decoupled from
> request. I'm in favor of the idea, I'm just not sure what solution makes
> the best sense yet (see above).
missed anything. Assuming I don't find anything new and interesting
(and assuming you're happy with my response on the API issues), this
should be able to land in trunk in the very near future.
Yours,
Russ Magee %-)
Tobias McNulty
I'll take another look at this patch next week to make sure I haven't
missed anything. Assuming I don't find anything new and interesting
(and assuming you're happy with my response on the API issues), this
should be able to land in trunk in the very near future.
Your API suggestions sound fine; at least, as long as what messages is doing import-wise isn't any worse than the rest of Django, it's fine with me. :)
Since this is a fairly significant change I would like to hear feedback from others before moving forward, in case there's something we missed. I updated the wiki to reflect the new proposed API:
http://code.djangoproject.com/wiki/SessionMessages#PotentialAPI
Once the final API changes have been made, I'll put up another patch, hopefully before next week.
Cheers,
Tobias
Luke Plant
> Your API suggestions sound fine; at least, as long as what messages
> is doing import-wise isn't any worse than the rest of Django, it's
> fine with me. :)
>
> Since this is a fairly significant change I would like to hear
> feedback from others before moving forward, in case there's
> something we missed. I updated the wiki to reflect the new
> proposed API:
issue, which was the fact that I had some code which only had access
to the User object, and not the request object, which had to be re-
plumbed. The only place this change will be an issue is where it is
difficult to do that (e.g. if your code is being called from somewhere
else which you can't change). There is no reason this should hold us
back - storing messages on the User is really a hack, and if people
have a problem with not being able to do it any more (i.e. in Django
1.4), they can use other hacks like storing the request in threadlocal
storage.
> Once the final API changes have been made, I'll put up another
> patch, hopefully before next week.
of patches, mainly very minor (attached as a mercurial bundle, use "hg
in fixes.bundle" to view, or if you give me commit rights to your
repository I'll commit directly), and one significant issue:
If I read the patch correctly, when a non-authenticated user uses a
generic view (create/update/delete), they will get a failure if the
messages framework is not installed. This is a big backwards
incompatibility.
The simplest solution is to catch the exception in the generic views.
Another option is to add a 'fail_silently' keyword argument,
defaulting to False, like the mail sending API. It would need to be
added to all the shortcut functions, and used in the generic views and
anywhere that you cannot rely on there being a current authenticated
User. Docs should include something about it being intended for re-
usable apps.
I'm leaning towards the latter solution — adding exception handling
makes a one-line call into a 4 line chunk of code, so the
fail_silently argument does make it significantly easier for the
framework to be used by re-usable apps.
Regards,
Luke
--
"Humiliation: The harder you try, the dumber you look."
(despair.com)
Luke Plant || http://lukeplant.me.uk/
SmileyChris
When Tobias reads this thread again, I'm sure he'll give you commit.
The fail_silently sounds good, and yes these failures were a rather
big oversight.
Tobias McNulty
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Tobias McNulty
I just converted some code, and the API seems good to me. I had one
issue, which was the fact that I had some code which only had access
to the User object, and not the request object, which had to be re-
plumbed. The only place this change will be an issue is where it is
difficult to do that (e.g. if your code is being called from somewhere
else which you can't change). There is no reason this should hold us
back - storing messages on the User is really a hack, and if people
have a problem with not being able to do it any more (i.e. in Django
1.4), they can use other hacks like storing the request in threadlocal
storage.
*gasp* Did I really just hear a core committer condoning threadlocals? Storing messages on the user was bad, but not that bad. :)
The simplest solution is to catch the exception in the generic views.
Another option is to add a 'fail_silently' keyword argument,
defaulting to False, like the mail sending API. It would need to be
added to all the shortcut functions, and used in the generic views and
anywhere that you cannot rely on there being a current authenticated
User. Docs should include something about it being intended for re-
usable apps.
Good catch. fail_silently sounds good to me. I'll add.
Tobias
Tobias McNulty
http://code.djangoproject.com/attachment/ticket/4604/django-contrib-messages-e4da706e1152.diff
Cheers,
Tobias
The simplest solution is to catch the exception in the generic views.
Another option is to add a 'fail_silently' keyword argument,
defaulting to False, like the mail sending API. It would need to be
added to all the shortcut functions, and used in the generic views and
anywhere that you cannot rely on there being a current authenticated
User. Docs should include something about it being intended for re-
usable apps.
Good catch. fail_silently sounds good to me. I'll add.
Alex Gaynor
>
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to
> django-develop...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/django-developers?hl=en.
>
:(. However, I just read through the diff that was uploaded, and I'm
curious why the decision to go with functions with APIs like
messages.info(request, msg). Instead of request.messages.info(msg).
The latter reads far better to me.
Thanks,
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Russell Keith-Magee
Firstly, there is a backwards compatibility problem. Legacy code won't
have the messages attribute on request, because it won't have the
messages middleware. Earlier versions of the code had a
'compat_add_message' function that had a fallback to using
request.user.message_set if request.messages wasn't available.
However, this would have meant introducing a function in 1.2 that was
deprecated at time of introduction - we would have needed to change
code in 1.2 to use the compatibility function, and change it again
when 1.4 came around.
I suggested that the compatibility function could actually be the
normal interface. That way, when 1.4 comes around, we can just update
the implementation of the 'add_message' function to remove the
deprecated code path.
The second reason is architectural. Using messages.info(request, msg)
decouples the process of sending messages from the message store on
the request itself. If we ever chose to change the way the message
store works, we're not bound to maintaining a request.messages data
structure.
I will admit that this second reason smells a little of YAGNI. I have
no idea what change would necessitate decoupling the message store
from request (other than the obvious reverse case of the
backwards-compatibility function). The backwards compatibility issue
is the primary reason for the change; the architectural reasoning is
very much "after the fact".
Yours,
Russ Magee %-)
Paul McLanahan
in the middleware that simply wrap the "real" functions? While I
understand the need for the others, and agree with their use
throughout the Django codebase, I also agree with Alex that they just
read better, and I'd prefer to use that API in my new projects.
Thanks,
Paul
Tobias McNulty
It's also a good thing, I realized last night, because it means one can use messages from a reusable app without requiring that all projects using the app enable messages.
Cheers,
Tobias
Sent from a mobile phone, please excuse any typos.
On Dec 3, 2009 6:25 AM, "Russell Keith-Magee" <freakb...@gmail.com> wrote:On Thu, Dec 3, 2009 at 2:39 PM, Alex Gaynor <alex....@gmail.com> wrote: > On Thu, Dec 3, 2009 at ...
-- You received this message because you are subscribed to the Google Groups "Django developers" g...
Waylan Limberg
> Latest patch, including fail_silently, is up for review:
>
> http://code.djangoproject.com/attachment/ticket/4604/django-contrib-messages-e4da706e1152.diff
>
get_messages function. What if a project has neither the
MessageMiddleware nor the contrib.auth app enabled? Sure a reusable
app could catch the error, but that error will change once all support
for user.messages are removed in 1.4. Shouldn't the call to
user.get_and_delete_messages be wrapped in a check that request.user
exists like the new add_message function does?
Tobias McNulty
Looking though this patch I couldn't help but notice the newget_messages function. What if a project has neither the
MessageMiddleware nor the contrib.auth app enabled? Sure a reusable
app could catch the error, but that error will change once all support
for user.messages are removed in 1.4. Shouldn't the call to
user.get_and_delete_messages be wrapped in a check that request.user
exists like the new add_message function does?
Luke Plant
> Looking though this patch I couldn't help but notice the new
> get_messages function. What if a project has neither the
> MessageMiddleware nor the contrib.auth app enabled? Sure a reusable
> app could catch the error, but that error will change once all
> support for user.messages are removed in 1.4. Shouldn't the call
> to user.get_and_delete_messages be wrapped in a check that
> request.user exists like the new add_message function does?
<<<
def get_messages(request):
"""
Returns the message storage on the request if it exists, otherwise
returns
user.message_set.all() as the old auth context processor did.
"""
if hasattr(request, '_messages'):
return request._messages
def get_user():
if hasattr(request, 'user'):
return request.user
else:
from django.contrib.auth.models import AnonymousUser
return AnonymousUser()
return lazy(memoize(get_user().get_and_delete_messages, {}, 0),
list)()
>>>
Luke
--
"I am going to let you move around more, just to break up the
mahogany." (True Quotes From Induhviduals, Scott Adams)
Luke Plant || http://lukeplant.me.uk/
Waylan Limberg
> On Thursday 03 December 2009 14:18:10 Waylan Limberg wrote:
>
>> Looking though this patch I couldn't help but notice the new
>> get_messages function. What if a project has neither the
>> MessageMiddleware nor the contrib.auth app enabled? Sure a reusable
>> app could catch the error, but that error will change once all
>> support for user.messages are removed in 1.4. Shouldn't the call
>> to user.get_and_delete_messages be wrapped in a check that
>> request.user exists like the new add_message function does?
>
> It is - the inline 'get_user()' function does effectively just that:
>
AnonymousUser returns an empty list. That was the part I missed.
Thanks for clearing that up.
Russell Keith-Magee
<freakb...@gmail.com> wrote:
> I'll take another look at this patch next week to make sure I haven't
> missed anything. Assuming I don't find anything new and interesting
> (and assuming you're happy with my response on the API issues), this
> should be able to land in trunk in the very near future.
* Why have all the tests migrated to the Django system tests? This is
a contrib app - the tests should be internal to the app.
* I'd rather the AddMessageFailure have a more generic name (like
MessageFailure). I don't see any need to be task specific in the
exception class name.
* I don't know why I didn't pick this up earlier, but the emerging
convention for pluggable backends is for the user to specify the name
of the module, and for the module to have a predictable class name.
i.e - each storage backend module should have a "StorageEngine" class,
and the user specifies the name of the module. There is also an
emerging convention to call the pluggable bits 'backends', rather than
picking a name like 'storage'. Net result: the user should be
configuring MESSAGE_BACKEND="django.contrib.messages.backends.user_messages"
rather than
MESSAGE_STORAGE =
'django.contrib.messages.storage.user_messages.LegacyFallbackStorage'
* The cookie backend is still doing a conditional import for json. The
django.utils.simplejson module already does this conditional import
check.
The rest of my notes are minor documentation fixes:
* The documentation for the message settings that modify old settings
(MIDDLEWARE_CLASSES, TEMPLATE_CONTEXT_PROCESSORS) should have a
..versionchanged note to indicate that they have changed in 1.2. This
has been done in the template API docs, but not in settings.txt.
* The deprecation note in topics/auth.txt says the new messages
framework should be used "whenever possible". This makes it sound
optional, when it isn't.
* The explanatory note in "Configuring the message engine" in the
message docs isn't required - or at least, isn't required until
*after* you have described the fact that storage backends are
configurable.
* The markup for the note about using standard message levels in
reusable apps should be marked up using ..note:: syntax, so it is
rendered as a breakout box.
* The explanatory note about overriding MESSAGE_LEVEL should be marked
up as an ..admonition:: , and should probably have an example to show
the correct technique.
Russ %-)
Luke Plant
> * I'd rather the AddMessageFailure have a more generic name (like
> MessageFailure). I don't see any need to be task specific in the
> exception class name.
only silences the error for the messages framework not being
installed. But there are a lot of other ways the messages backend
could potentially fail. Should we silence those errors too? I would
suggest not, or there should be a separate keyword argument for that.
But this should be clear in the docs.
> * I don't know why I didn't pick this up earlier, but the emerging
> convention for pluggable backends is for the user to specify the
> name of the module, and for the module to have a predictable class
> name. i.e - each storage backend module should have a
> "StorageEngine" class, and the user specifies the name of the
> module. There is also an emerging convention to call the pluggable
> bits 'backends', rather than picking a name like 'storage'. Net
> result: the user should be configuring
> MESSAGE_BACKEND="django.contrib.messages.backends.user_messages"
> rather than
> MESSAGE_STORAGE =
> 'django.contrib.messages.storage.user_messages.LegacyFallbackStorag
> e'
it (db, session, cache) and the e-mail stuff just added uses this
convention. There are other established conventions in the code base,
some of which are more recent IIRC:
* that used by the file storage classes, file upload handlers and the
authentication backend where the specific class is referred to.
* that used by the template loaders, where the specific callable
is referred to.
I actually much prefer the explicit naming of a class/callable. Here
are some arguments in favour of this method, some of which are
stronger than others:
* you can put multiple backends in one file if you want to, and you
are not forced to duplicate imports or get creative with module
names if you happen to have several backends that really belong
in the same module.
* it is much more obvious for someone who does not know the code,
since it doesn't rely on a convention of a class with a certain
name. If they see the setting for MESSAGE_STORAGE pointing to
a class, they will immediately know where the implementation is,
whereas if it points to a module and the module contains multiple
classes, they will have to guess which class is the main one, or
browse docs/source code.
* it makes it less verbose and confusing if one backend refers to
another (as happens with messages), because all the classes
have different names, rather than having the same name. We
currently have:
from django.contrib.messages.storage.fallback import FallbackStorage
...
class LegacyFallbackStorage(FallbackStorage):
storage_classes = (UserMessagesStorage,) +
FallbackStorage.storage_classes
which would need to be changed to:
from django.contrib.messages.backends import fallback
...
class StorageEngine(fallback.StorageEngine):
storage_classes = (UserMessageStorageEngine,) +
fallback.StorageEngine.storage_classes
or, using 'as' for the import:
from django.contrib.messages.backends.fallback import StorageEngine \
as FallbackStorageEngine
...
class StorageEngine(FallbackStorageEngine):
storage_classes = (UserMessageStorageEngine,) +
FallbackStorageEngine.storage_classes
(the fallback storage module would have similar issues)
The DB subsystem, which uses backends, is big and has multiple
classes, so it makes sense for it to be referred to by module names.
But if there is exactly one class which implements a feature, I don't
see the sense in having a setting that refers to the containing
module. There might be a future proofing argument here, but for
smaller features it sounds like YAGNI to me.
For the cache subsystem, the name of the module (without the 'path')
is used as the protocol part of a URI-style setting, so it also makes
sense for it to be short and lower case.
As for the e-mail and session subsystems, IMO they are using the less
preferable convention. Some of the disadvantages mentioned above don't
exist for the e-mail backends, because they don't use each other like
the messages ones do.
Regards,
Luke
--
"Idiocy: Never underestimate the power of stupid people in large
groups." (despair.com)
Luke Plant || http://lukeplant.me.uk/
Tobias McNulty
<freakb...@gmail.com> wrote:
> * Why have all the tests migrated to the Django system tests? This is
> a contrib app - the tests should be internal to the app.
with a test urls.py and views.py that make sure all the plumbing works
together. In short, I wanted a way to make use of the test Client
without including the dummy code in the distribution.
I think it has also been discussed elsewhere; that some would prefer
not to deploy the tests with the production application - one more
reason to keep them in 'tests' instead of in the contrib app.
From what I can tell, this seems to be in line with what some of the
other contrib apps are doing, such as admin and comments.
> * I'd rather the AddMessageFailure have a more generic name (like
> MessageFailure). I don't see any need to be task specific in the
> exception class name.
> * I don't know why I didn't pick this up earlier, but the emerging
> convention for pluggable backends is for the user to specify the name
> of the module, and for the module to have a predictable class name.
> i.e - each storage backend module should have a "StorageEngine" class,
> and the user specifies the name of the module. There is also an
> emerging convention to call the pluggable bits 'backends', rather than
> picking a name like 'storage'. Net result: the user should be
> configuring MESSAGE_BACKEND="django.contrib.messages.backends.user_messages"
> rather than
> MESSAGE_STORAGE =
> 'django.contrib.messages.storage.user_messages.LegacyFallbackStorage'
http://code.djangoproject.com/browser/django/trunk/django/core/files/storage.py
> * The cookie backend is still doing a conditional import for json. The
> django.utils.simplejson module already does this conditional import
> check.
> The rest of my notes are minor documentation fixes:
>
> * The documentation for the message settings that modify old settings
> (MIDDLEWARE_CLASSES, TEMPLATE_CONTEXT_PROCESSORS) should have a
> ..versionchanged note to indicate that they have changed in 1.2. This
> has been done in the template API docs, but not in settings.txt.
> * The deprecation note in topics/auth.txt says the new messages
> framework should be used "whenever possible". This makes it sound
> optional, when it isn't.
> * The explanatory note in "Configuring the message engine" in the
> message docs isn't required - or at least, isn't required until
> *after* you have described the fact that storage backends are
> configurable.
> * The markup for the note about using standard message levels in
> reusable apps should be marked up using ..note:: syntax, so it is
> rendered as a breakout box.
> * The explanatory note about overriding MESSAGE_LEVEL should be marked
> up as an ..admonition:: , and should probably have an example to show
> the correct technique.
Other than the question about renaming storage -> backend I think we
are close to prime time. Let me know what you think.
Tobias McNulty
backend, so thanks for picking this up Luke.
I don't have much to add to your argument except to say that it would
be non-trivial to move to a more strictly organized/named setup.
Cheers,
Tobias
>
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>
Tobias McNulty
uploaded a new patch here:
http://code.djangoproject.com/attachment/ticket/4604/django-contrib-messages-6399c12d1773.diff
Luke, it includes a note that fail_silently only hides the error when
messages is disabled.
I'm going to give this a final whirl by converting a project or two.
Cheers,
Tobias
Russell Keith-Magee
> On Sat, Dec 5, 2009 at 8:40 AM, Russell Keith-Magee
> <freakb...@gmail.com> wrote:
>> * Why have all the tests migrated to the Django system tests? This is
>> a contrib app - the tests should be internal to the app.
>
> They were moved to facilitate inclusion of a more complete test suite
> with a test urls.py and views.py that make sure all the plumbing works
> together. In short, I wanted a way to make use of the test Client
> without including the dummy code in the distribution.
contrib.auth tests; they define both views and urls that are only used
during the testing process.
> I think it has also been discussed elsewhere; that some would prefer
> not to deploy the tests with the production application - one more
> reason to keep them in 'tests' instead of in the contrib app.
deploying anything that can help you validate that the code you're
deploying actually works as intended. There is a bigger argument here
about application and integration testing, which has been raised
previously [1].
[1] http://groups.google.com/group/django-developers/msg/dba95d9702b8b5cd
> From what I can tell, this seems to be in line with what some of the
> other contrib apps are doing, such as admin and comments.
for a different reason - they require test models. i.e., you can't
display an admin interface unless you have models to display. Django's
app-level testing facilities don't provide for test models (yet - see
#7835), so the admin and comments tests need to be in the system test
area.
>> * I don't know why I didn't pick this up earlier, but the emerging
>> convention for pluggable backends is for the user to specify the name
>> of the module, and for the module to have a predictable class name.
>> i.e - each storage backend module should have a "StorageEngine" class,
>> and the user specifies the name of the module. There is also an
>> emerging convention to call the pluggable bits 'backends', rather than
>> picking a name like 'storage'. Net result: the user should be
>> configuring MESSAGE_BACKEND="django.contrib.messages.backends.user_messages"
>> rather than
>> MESSAGE_STORAGE =
>> 'django.contrib.messages.storage.user_messages.LegacyFallbackStorage'
>
> Really? Files definitely seem to be more on the "storage" side of things:
>
> http://code.djangoproject.com/browser/django/trunk/django/core/files/storage.py
introduced by history. Yes, files and templates use function/class
level specifiers, but db, session, cache, and email all use module
level specifiers. Module level specifiers have the majority at this
point, and personally, I prefer the module approach - it shortens the
user configuration strings, and it enforces good usage of modules.
My point is that I think we should pick an approach and stick with it.
Long term, we should aim to be consistent. For example, #6262 gives us
the opportunity to normalize template loaders into a common scheme.
> Other than the question about renaming storage -> backend I think we
> are close to prime time. Let me know what you think.
both require code changes, they aren't major code changes. Modulo
those changes, I'm happy.
Yours,
Russ Magee %-)
Luke Plant
> > Really? Files definitely seem to be more on the "storage" side
> > of things:
> >
> > http://code.djangoproject.com/browser/django/trunk/django/core/fi
> >les/storage.py
>
> The problem we have here is that we have all sorts of
> inconsistencies introduced by history. Yes, files and templates
> use function/class level specifiers, but db, session, cache, and
> email all use module level specifiers. Module level specifiers
> have the majority at this point, and personally, I prefer the
> module approach - it shortens the user configuration strings, and
> it enforces good usage of modules.
enforces a *single* way of using modules, that is, one class per
module. That is not necessarily 'good', it certainly isn't Pythonic
in my experience, and it could well be bad. As such, it would be
opinionated in a bad way - it forces other developers to have a
proliferation of modules which might be very inconvenient. Why not
just leave the decision in the hands of the people who are writing and
organizing the code? We are all consenting adults here.
In addition to the arguments in my other message, you also have the
case where some third party writes a backend, and then realises they
need another. So, they had:
acme.messages.MessageStorage
and just want to add
acme.messages.SuperMessageStorage
Instead, they are forced to move code around so each of these can live
in its own module, or else have stub modules that serve no purpose
except to appease the Django-gods.
I agree that it would be good to be consistent if possible, but I
would also like to see the better convention used, and the e-mail
system could just as easily be changed as the messages system at this
point, which would swing the majority the other way (it's actually
currently "4-all" by my count, and that doesn't include the other
configurable callables and classes, such as TEST_RUNNER,
CSRF_FAILURE_VIEW, context processors, middlewares and view functions,
which all use full paths). And #6262 could also go the other way too.
Moving forward, we do need *both* methods. The database stuff
involves various classes and submodules, requiring the top module to
be specified and many callables and classes need to be specified using
full dotted paths (unless you want a separate module for every context
processor...). So then we need to decide which one to standardise on.
Backwards compatibility is possible either way around - if we wanted
to change, for example, the session storage setting to being a full
path, it would be easy - if the path supplied gets you to a module,
just look for 'SessionStorage' inside that module and you are done,
otherwise use the thing you've got (or something similar). I think
going the other way is also possible, with a similar level of
fragility.