Re: decorator_from_middleware change

[Luke Plant]

On Saturday 26 September 2009 20:10:59 kmike wrote:

> Not true. cache_page decorator is now documented as putting view
>  first and timeout second. Take a look at
>  http://docs.djangoproject.com/en/dev/topics/cache/#the-per-view-ca
> che

Doh, I don't know how I managed to read those several times and not 
see that!  I've fixed it now, thanks!  

Alex Gaynor reckoned both ways round were possible with previous code.  
I haven't checked that, but I'll believe him, and I've added 
compatibility for that.

> Another thought. If key_prefix is deprecated in cache_page
>  decorator maybe it should be also deprecated in CacheMiddleware?

It's not 'deprecated' - it was never documented, and therefore never a 
feature in the first place, and so I didn't see the need to implement 
it.

We could of course re-add it, and document it.  As it happens, doing 
so looks easy.  I'm not sure why those arguments are not documented 
anywhere, either for cache_page or CacheMiddleware, that's why I'm 
reluctant to add this -- we would then be committing ourselves to 
supporting something that we might have wanted to remove.  Putting 
specific support for it in cache_page would make it harder to remove.

Does anyone else know if we want to expose all the arguments of 
CacheMiddleware in cache_page?  I can easily add them if so.

Regards,

Luke

-- 
"Smoking cures weight problems...eventually..." (Steven Wright)

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[kmike]

If you search for `cache_page` decorator than it becomes clear that
traditional approach is to use it in now unsupported way:

myview = cache_page(myviewt, timeout)

For example here:

http://www.google.com/codesearch?hl=ru=N=%22+%3D+cache_page%22++lang:python=rr_r=lang:python

nobody uses `myview = cache_page(timeout, myview)` syntax.

So I suppose this change can broke a lot of third-party apps, for
example django-robots and django-mediafiles.

> I don't know for certain how many people may be using this idiom,
since it was never documented (AFAICT cache_page always documented
itself as putting the timeout first and the view function second), so
I'm not really sure what (if anything) we should do about it.

Not true. cache_page decorator is now documented as putting view first
and timeout second. Take a look at 
http://docs.djangoproject.com/en/dev/topics/cache/#the-per-view-cache

Another thought. If key_prefix is deprecated in cache_page decorator
maybe it should be also deprecated in CacheMiddleware?

On 24 сен, 05:50, Luke Plant  wrote:
> On Monday 21 September 2009 20:27:50 Jacob Kaplan-Moss wrote:
>
> > No, I think this is precisely correct. I've been meaning to do
> >  exactly what you're proposing for a while myself; just haven't
> >  gotten around to it.
>
> > > decorator_from_middleware isn't actually documented anywhere
>
> > I actually avoided documenting it because it's broken. Once you fix
> > it, we should (i.e. I will, if you don't have time) document it.
>
> OK, it's committed now. (r11586, r11593)
>
> Technically this is a bug fix (#6371), so it ought to get backported
> to 1.1.X.  However, it does actually introduce backwards
> incompatibilities with cache_page (cache_page still works exactly as
> documented, but various people were using various undocumented
> features of it).  Also, you could argue it is a new feature - "these
> decorators now work with methods".
>
> So, in light of those things, should it be backported to 1.1.X or not?
>
> Luke
>
> --
> "Pretension: The downside of being better than everyone else is
> that people tend to assume you're pretentious." (despair.com)
>
> Luke Plant ||http://lukeplant.me.uk/
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Luke Plant]

On Monday 21 September 2009 20:27:50 Jacob Kaplan-Moss wrote:

> No, I think this is precisely correct. I've been meaning to do
>  exactly what you're proposing for a while myself; just haven't
>  gotten around to it.
> 
> > decorator_from_middleware isn't actually documented anywhere
> 
> I actually avoided documenting it because it's broken. Once you fix
> it, we should (i.e. I will, if you don't have time) document it.

OK, it's committed now. (r11586, r11593)

Technically this is a bug fix (#6371), so it ought to get backported 
to 1.1.X.  However, it does actually introduce backwards 
incompatibilities with cache_page (cache_page still works exactly as 
documented, but various people were using various undocumented 
features of it).  Also, you could argue it is a new feature - "these 
decorators now work with methods".

So, in light of those things, should it be backported to 1.1.X or not?

Luke

-- 
"Pretension: The downside of being better than everyone else is 
that people tend to assume you're pretentious." (despair.com)

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Luke Plant]

On Wednesday 23 September 2009 23:40:25 James Bennett wrote:

> So, I've worked out what the problem is.
> 
> Previously either of these worked:
> 
> cache_page(timeout, view)
> cache_page(view, timeout)
> 
> Now, cache_page assumes that the first positional argument will be
>  the timeout. So what we're seeing when running some of our code on
>  trunk is cache_page treating the timeout value (an integer) as the
>  view to be cached, and failing when trying to find things like
>  __module__ attached to it.
> 
> I don't know for certain how many people may be using this idiom,
> since it was never documented (AFAICT cache_page always documented
> itself as putting the timeout first and the view function second),
>  so I'm not really sure what (if anything) we should do about it.
> 
> There may also be deeper issues with the fact that it's not
>  possible to call cache_page with explicit keyword args, since it
>  accepts **kwargs but doesn't do anything with it, always assuming
>  that the timeout and view can be pulled from positional args.

I deliberately wrote it the way it is documented, and only that way. 
So it accepts these forms:

cache_page(timeout, view)
cache_page(timeout)(view)

It shouldn't accept **kwargs though, that's a mistake, because it does 
nothing with them, and it would be better to fail loudly in that case.

I've fixed that, and also added some asserts to get a better error 
message for now unsupported ways of calling it. Do you think this is 
enough?

Luke

-- 
"Pretension: The downside of being better than everyone else is 
that people tend to assume you're pretentious." (despair.com)

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[James Bennett]

On Tue, Sep 22, 2009 at 4:07 PM, Luke Plant  wrote:
> Bummer, I tried hard not to break it - I added backwards
> compatibility tests for the basic different uses.  Could you produce
> a test case?

So, I've worked out what the problem is.

Previously either of these worked:

cache_page(timeout, view)
cache_page(view, timeout)

Now, cache_page assumes that the first positional argument will be the
timeout. So what we're seeing when running some of our code on trunk
is cache_page treating the timeout value (an integer) as the view to
be cached, and failing when trying to find things like __module__
attached to it.

I don't know for certain how many people may be using this idiom,
since it was never documented (AFAICT cache_page always documented
itself as putting the timeout first and the view function second), so
I'm not really sure what (if anything) we should do about it.

There may also be deeper issues with the fact that it's not possible
to call cache_page with explicit keyword args, since it accepts
**kwargs but doesn't do anything with it, always assuming that the
timeout and view can be pulled from positional args.


-- 
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Alex Gaynor]

On Tue, Sep 22, 2009 at 6:04 PM, Jacob Kaplan-Moss  wrote:
>
> On Tue, Sep 22, 2009 at 4:16 PM, Luke Plant  wrote:
>> James B - do we have a place to list things like this i.e. things
>> that probably should go in release notes?
>
> I think it'd probably be best to just start
> docs/releases/1.2-alpha.txt right now. We can list this stuff as we
> add it, and then someone (probably James or I) can clean it up into a
> solid doc in time for the release.
>
> Jacob
>
> >
>

Note that we now also have the:
http://docs.djangoproject.com/en/dev/internals/deprecation/#internals-deprecation
page for documenting depercated items.

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

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Jacob Kaplan-Moss]

On Tue, Sep 22, 2009 at 4:16 PM, Luke Plant  wrote:
> James B - do we have a place to list things like this i.e. things
> that probably should go in release notes?

I think it'd probably be best to just start
docs/releases/1.2-alpha.txt right now. We can list this stuff as we
add it, and then someone (probably James or I) can clean it up into a
solid doc in time for the release.

Jacob

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[kmike]

Regarding key_prefix parameter: it's all simply about that:
http://code.djangoproject.com/ticket/11269
My proposal was to move things in opposite direction: to promote
`key_prefix` parameter, document it and make it more useful. If it is
an 'Design decision needed'-type of issue and design decision is made
then OK :)


On 23 сен, 03:16, Luke Plant  wrote:
> On Tuesday 22 September 2009 20:37:05 kmike wrote:
>
> > cache_page decorator previously used to have optional
> >  'key_prefix' argument, not only timeout. Is it gone? Can I use
>
> > @cache_page(3600, key_prefix='vasia')
> > def my_func(request)
> >    ...
>
> That wasn't documented anywhere as far as I can see, so now it's
> gone. I had to make some decision about how far the backwards
> compatibility went, and current policy is to go with documented API,
> which explicitly says "cache_page takes a single argument: the cache
> timeout, in seconds." [1]
>
> If there is some documentation/tutorial about the key_prefix
> argument, or if this change is going to cause lots of breakage,
> perhaps we'll have to revisit this.
>
> For the time being, you can create a 'cache_page' decorator that
> does what you want very easily, as in the example below.
>
> James B - do we have a place to list things like this i.e. things
> that probably should go in release notes?
>
> > Another question: in 'decorator_from_middleware_with_args'
> >  docstring example stated:
> > Use like::
>
> >     cache_page = decorator_from_middleware(CacheMiddleware)
>
> > Is it correct?
>
> Nope, should be decorator_from_middleware_with_args of course,
> thanks, will fix.
>
> Luke
>
> [1]http://docs.djangoproject.com/en/dev/topics/cache/#the-per-view-
> cache
>
> --
> "Pessimism: Every dark cloud has a silver lining, but lightning
> kills
> hundreds of people each year trying to find it." (despair.com)
>
> Luke Plant ||http://lukeplant.me.uk/
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Luke Plant]

On Tuesday 22 September 2009 20:37:05 kmike wrote:
> cache_page decorator previously used to have optional
>  'key_prefix' argument, not only timeout. Is it gone? Can I use
> 
> @cache_page(3600, key_prefix='vasia')
> def my_func(request)
>...
> 

That wasn't documented anywhere as far as I can see, so now it's 
gone. I had to make some decision about how far the backwards 
compatibility went, and current policy is to go with documented API, 
which explicitly says "cache_page takes a single argument: the cache 
timeout, in seconds." [1]

If there is some documentation/tutorial about the key_prefix 
argument, or if this change is going to cause lots of breakage, 
perhaps we'll have to revisit this.

For the time being, you can create a 'cache_page' decorator that 
does what you want very easily, as in the example below.

James B - do we have a place to list things like this i.e. things 
that probably should go in release notes?

> Another question: in 'decorator_from_middleware_with_args'
>  docstring example stated:
> Use like::
> 
> cache_page = decorator_from_middleware(CacheMiddleware)
> 
> Is it correct?

Nope, should be decorator_from_middleware_with_args of course, 
thanks, will fix.

Luke

[1] http://docs.djangoproject.com/en/dev/topics/cache/#the-per-view-
cache


-- 
"Pessimism: Every dark cloud has a silver lining, but lightning 
kills 
hundreds of people each year trying to find it." (despair.com)

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Luke Plant]

On Tuesday 22 September 2009 21:31:08 James Bennett wrote:
> On Mon, Sep 21, 2009 at 9:04 PM, Luke Plant 
<l.plant...@cantab.net> wrote:
> > I've committed my change [1], and also replaced _CheckLogin
> > with my method [2] (it was essentially the same method, just
> > generalised).
> 
> The decorator_from_middleware change appears to have broken
> cache_page; I'm now getting "AttributeError: 'int' object has no
> attribute '__module__'" from views which use cache_page.

Bummer, I tried hard not to break it - I added backwards 
compatibility tests for the basic different uses.  Could you produce 
a test case?

Cheers,

Luke 

-- 
"Pessimism: Every dark cloud has a silver lining, but lightning 
kills hundreds of people each year trying to find it." (despair.com)

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[James Bennett]

On Mon, Sep 21, 2009 at 9:04 PM, Luke Plant <l.plant...@cantab.net> wrote:
> I've committed my change [1], and also replaced _CheckLogin with my method [2]
> (it was essentially the same method, just generalised).

The decorator_from_middleware change appears to have broken
cache_page; I'm now getting "AttributeError: 'int' object has no
attribute '__module__'" from views which use cache_page.


-- 
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[kmike]

cache_page decorator previously used to have optional 'key_prefix'
argument, not only timeout. Is it gone? Can I use

@cache_page(3600, key_prefix='vasia')
def my_func(request)
   ...

Another question: in 'decorator_from_middleware_with_args' docstring
example stated:
Use like::

cache_page = decorator_from_middleware(CacheMiddleware)

Is it correct?

Or maybe I've missed something?

Please excuse my English :)

On 22 сен, 08:04, Luke Plant  wrote:
> On Monday 21 September 2009 20:27:50 Jacob Kaplan-Moss wrote:
>
> > On Mon, Sep 21, 2009 at 1:21 PM, Luke Plant  wrote:
> > > However, decorator_from_middleware is a pain, since it doesn't always
> > > return a an actual decorator, for "historical reasons".  I need to change
> > > this to fix the bug.  Is anyone against this?
>
> > No, I think this is precisely correct. I've been meaning to do exactly
> > what you're proposing for a while myself; just haven't gotten around
> > to it.
>
> > > decorator_from_middleware isn't actually documented anywhere
>
> > I actually avoided documenting it because it's broken. Once you fix
> > it, we should (i.e. I will, if you don't have time) document it.
>
> I've committed my change [1], and also replaced _CheckLogin with my method [2]
> (it was essentially the same method, just generalised).
>
> I haven't added any docs, because I'm not sure it's perfect yet (and also
> because I am lazy and I was mainly working on something else, this just got in
> the way).  In the 'normal' case where you don't need to pass any args to the
> middleware, it is exactly the same (but works for decorating methods now).  In
> the abnormal case, you have to use decorator_from_middleware_with_args, which
> is a pretty ugly name, if explicit.  I don't know if you had other ideas about
> what to do with this.  Changing that name is easy, it's only used by
> cache_page.
>
> Luke
>
> [1]http://code.djangoproject.com/changeset/11586
> [2]http://code.djangoproject.com/changeset/11587
>
> --
> Parenthetical remarks (however relevant) are unnecessary
>
> Luke Plant ||http://lukeplant.me.uk/
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Luke Plant]

On Monday 21 September 2009 20:27:50 Jacob Kaplan-Moss wrote:
> On Mon, Sep 21, 2009 at 1:21 PM, Luke Plant  wrote:
> > However, decorator_from_middleware is a pain, since it doesn't always
> > return a an actual decorator, for "historical reasons".  I need to change
> > this to fix the bug.  Is anyone against this?
> 
> No, I think this is precisely correct. I've been meaning to do exactly
> what you're proposing for a while myself; just haven't gotten around
> to it.
> 
> > decorator_from_middleware isn't actually documented anywhere
> 
> I actually avoided documenting it because it's broken. Once you fix
> it, we should (i.e. I will, if you don't have time) document it.

I've committed my change [1], and also replaced _CheckLogin with my method [2]
(it was essentially the same method, just generalised).

I haven't added any docs, because I'm not sure it's perfect yet (and also 
because I am lazy and I was mainly working on something else, this just got in 
the way).  In the 'normal' case where you don't need to pass any args to the 
middleware, it is exactly the same (but works for decorating methods now).  In 
the abnormal case, you have to use decorator_from_middleware_with_args, which 
is a pretty ugly name, if explicit.  I don't know if you had other ideas about 
what to do with this.  Changing that name is easy, it's only used by 
cache_page.

Luke

[1] http://code.djangoproject.com/changeset/11586
[2] http://code.djangoproject.com/changeset/11587

-- 
Parenthetical remarks (however relevant) are unnecessary

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

Re: decorator_from_middleware change

[Jacob Kaplan-Moss]

On Mon, Sep 21, 2009 at 1:21 PM, Luke Plant  wrote:
> However, decorator_from_middleware is a pain, since it doesn't always return a
> an actual decorator, for "historical reasons".  I need to change this to fix
> the bug.  Is anyone against this?

No, I think this is precisely correct. I've been meaning to do exactly
what you're proposing for a while myself; just haven't gotten around
to it.

> decorator_from_middleware isn't actually documented anywhere

I actually avoided documenting it because it's broken. Once you fix
it, we should (i.e. I will, if you don't have time) document it.

Jacob

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---

decorator_from_middleware change

[Luke Plant]

Hi all,

I need to fix #6371 [1], and I've found a nice way to do it (something like 
[2]).

However, decorator_from_middleware is a pain, since it doesn't always return a 
an actual decorator, for "historical reasons".  I need to change this to fix 
the bug.  Is anyone against this?

decorator_from_middleware isn't actually documented anywhere, but I'm guessing 
it might be used outside of the Django code base.  I want to change it as 
follows.

For decorators like gzip_page, everything works as normal:

  gzip_page = decorator_from_middleware(GzipMiddleware)

The only difference is that gzip_page is now a true decorator.

For decorators which require arguments given to the middleware, you will now 
need to use decorator_from_middleware_with_args.

I will manually fix the one decorator this breaks (cache_page) to provide 
backwards compatibility with the published documentation.

Does anyone object to this change?  It makes fixing #6371 a *lot* easier.  In 
fact I can't actually get my head around how to fix it at all without the 
change.

Luke


[1] http://code.djangoproject.com/ticket/6371
[2] http://stackoverflow.com/questions/1288498/using-the-same-decorator-with-
arguments-with-functions-and-methods/1288936#1288936

-- 
Parenthetical remarks (however relevant) are unnecessary

Luke Plant || http://lukeplant.me.uk/

--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@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
-~--~~~~--~~--~--~---