New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: BeforeTraversal #2469

Merged
merged 14 commits into from Apr 13, 2016

Conversation

Projects
None yet
4 participants
@bertjwregeer
Member

bertjwregeer commented Apr 11, 2016

This updates #1876 and renames it from RouteFound to BeforeTraversal.

bertjwregeer added some commits Apr 11, 2016

Move event to the appropriate location
This way the notification is not sent only after finding a route, but
anytime generically before attempting traversal.
@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

@stevepiercy This may require your expertise in massaging the router.rst document to make it all flow correctly. Currently 7 and 8 both have information about the BeforeTraversal event because it is emitted BEFORE the root_factory is called.

Not sure how to best word that.

bertjwregeer added some commits Apr 11, 2016

Add explicit tests for IBeforeTraversal/BeforeTraversal
Although the new code was already being covered by other tests, this
adds some explicit testing to make sure it all works.
@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

@mmerickel let me know if there is anything else that needs to be done here

@dstufft Please let me know if this still matches the spirit of your original idea.

@stevepiercy

This comment has been minimized.

Member

stevepiercy commented Apr 11, 2016

@bertjwregeer how about this? I'm a little uncertain about the third item.

#. If any route matches, the route mapper adds the attributes ``matchdict``
   and ``matched_route`` to the request object. The former contains a
   dictionary representing the matched dynamic elements of the request's
   ``PATH_INFO`` value, and the latter contains the
   :class:`~pyramid.interfaces.IRoute` object representing the route which
   matched.

#. A :class:`~pyramid.events.BeforeTraversal` :term:`event` is sent to any
   subscribers.

#. Continuing, if any route matches, the root object associated with the found
   route is generated. If the :term:`route configuration` which matched has an
   associated ``factory`` argument, then this factory is used to generate the
   root object; otherwise a default :term:`root factory` is used.

   However, if no route matches, and if a ``root_factory`` argument was passed
   to the :term:`Configurator` constructor, that callable is used to generate
   the root object. If the ``root_factory`` argument passed to the
   Configurator constructor was ``None``, a default root factory is used to
   generate a root object.

Additionally, I think I need to add a purple box for the new event, BeforeTraversal, and place it just to the right of the green URL dispatch box in the Request Processing diagram. Please let me know, and I will make it so.

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

@bertjwregeer I think it might be nice to expose the root to the BeforeTraversal event by moving the event down a couple lines. Maybe not but it's a thought. It's hard to say without a real use-case for the event.

@stevepiercy #2413 also needs an update to the diagram... you might just want to make a separate issue to update the diagram once prior to release.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Apr 11, 2016

My original use case was to give me a hook that happened prior to anything that would touch the database. In my case it was via traversal that the DB was getting touched and I think that would mean, to satisfy the original use case, BeforeTraversal would need to happen prior to the root being created.

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

Yeah but a requirement like "hitting the database" is completely application dependent obviously. Is there a reason you aren't using a separate read-only db session for those requests and then just performing a rollback on any changes to the writable session? That's the approach I'd take so just curious.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Apr 11, 2016

@mmerickel Duplication, particularly in that if I want to have the same resource work for both read only and writable sessions it means I need to duplicate my factories so that I have one for read only and one for writable or I need some sort of wrapper around them, but that just sort of feels wrong to me. Whether or not my session is writable has nothing to do with how I traverse the object graph to locate the final resource. It's an error in the separation of concerns IMO.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

@mmerickel I moved it before the root being created because the root may or may not touch the database, and IMHO creation of the root is the start of traversal.

I had moved it to after the root creation, but quickly realised that would defeat one of the goals that @dstufft had in creating the original RouteFound event. It is also the reason I added the comment above it, with the hopes that would stop someone from in the future going "Hey, I can move this down after root factory creation".

@stevepiercy That looks good actually. Could you create a PR on the feature/BeforeTraversal branch with that change? I'll merge it :-)

@stevepiercy stevepiercy referenced this pull request Apr 11, 2016

Closed

update Pyramid Request Processing diagram #2473

2 of 2 tasks complete
Merge pull request #2475 from stevepiercy/feature/BeforeTraversal
update BeforeTraversal event in router.rst
@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 12, 2016

LGTM, I'll let it sit for a bit incase there are some more comments.

@mmerickel mmerickel merged commit b1527e7 into master Apr 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment