On Tue, Nov 17, 2009 at 9:28 PM, Tobias McNulty <tob...@caktusgroup.com> wrote:
> On Thu, Nov 5, 2009 at 9:15 AM, Russell Keith-Magee <freakboy3...@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 :-)
Here goes. Although there's a lot here, most of the problems are minor
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 %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=.
