Hi Russell,
Thanks for all your feedback. I think I've got everything covered except
for a few questions about the API (see below).
On Fri, Nov 20, 2009 at 3:29 AM, Russell Keith-Magee <freakboy3...@gmail.com
> wrote:
> * 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.,:
>
> 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 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).
Cheers,
Tobias
--
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com
--
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=en.
