an empty request.finished_callbacks returns None in master/1.6; returns empty tuple () in earlier versions

[jvanasco]

This raised an exception for me when testing against #1534

I confirmed this behavior in a fresh virtualenv against 1.5.4 and and master/1.6.

I couldn't figure out where the change comes from. It's not documented, and will break anything that inspects request.finished_callbacks without checking for None (which was previously not needed)

[tseaver]

request.finished_callbacks is an attribute, not a method (so it doesn't "return" anything). By inspection, it can only be the empty tuple (on initialization) or a list (after a callback has been added); that list gets emptied during request._process_finished_callbacks(). We have tests that assert those values.

Please provide some kind of example which shows the behavior you are seeing.

[jvanasco]

It used to be an empty tuple on initialization. It is now None. The tests pass, because the test is checking for it to initialize as None (see line 174).

This is a departure from earlier Pyramid releases, where it initialized as a tuple (iterable!).

in 1.5.4 and prior, request.finished_callbacks initializes to an empty tuple, and is replaced by a list.

in 1.6/master, it initializes to None, and is replaced by a collections.deque list-like collection.

Here's a drop-in replacement view for the starter template https://gist.github.com/jvanasco/d0b8ba106291f40139e4

[jvanasco]

I just wanted to add - I have no opinion on which behavior is better. This should just be documented as a potentially-breaking change if this new behavior is intended.

[mmerickel]

I'm pretty sure request.xxx_callbacks are not public APIs. At least I can't find them anywhere in the docs. I'm not against fixing the issue but just saying.

[tseaver]

@mmerickel the switch to None is in response to your comment on #1373.

[mmerickel]

@tseaver oh I'm aware, I didn't realize anyone was depending on the internal API.

[jvanasco]

Interesting. I didn't realize it was an internal API.

I use it in two situations -- read only:

• Test Suites (90% of my usage). Check to see if the includeme successfully runs and registers the appropriate add_finished_callback methods.

• One library I wrote has an "ensure_cleanup" function. It checks to see if the cleanup function has already been added, as It only needs to be run once. I was naively relying on that.

I think it's fine to leave as-is. A note in the changelog would be great though, because I expect others who have written packages have relied on this being an empty list.

Searching in github, it looks like "response_callbacks" is more popular (also an internal api, it seems), but is used a lot in testing. Quickly searching github's source, the two most popular packages I saw that expected an iterable for that were pyramid_redis_sessions and pyramid_zodbconn

[mmerickel]

If we change this I'd advocate for something closer to the following:

class Request(object):
    _finished_callbacks = None

    @property
    def finished_callbacks(self):
        callbacks = getattr(self, '_finished_callbacks', None)
        if callbacks is None:
            callbacks = collections.deque()
            self._finished_callbacks = callbacks
        return callbacks

I see no problem with documenting it as a read-only iterable but the fact that it is a deque should be internal.

[mmerickel]

I've updated this to return an empty deque. It can now be checked for a length of 0.