Skip to content
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

added support for a default_action to config.add_handler #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jvanasco
Copy link

I added support for a "default_action" kwarg to add_handlers

This allows the following two config lines to be collapsed into one:

was:
config.add_handler("test", "/test/", "app.handlers.test:Test", action="index")
config.add_handler("test", "/test/{action}", "app.handlers.test:Test")

now:
config.add_handler("test", "/test/{action}", "app.handlers.test:Test", default_action="index")

it does this by implicitly creating another route named "test-default_action" , removing the ":action" args from the pattern, and subbing in "default_action" for "action"

I wasn't sure how the inheritance worked, so I used "add_handler(self" instead of "self.add_handler(".

@mmerickel
Copy link
Member

So how do I generate a url for this? The only way I see is request.route_url('route-default-action'), which will require documentation update, and special-casing code in views/templates the same as you do now. All in the name of saving 1 line of configuration code.

There is also a bug in your patch for config.add_handler('test', '/test/{action}/') the resulting route will be /test// for test-default-action.

Anyway, I'm -1 on this feature, it was even removed from Pylons (routes minimization=False) because it's "magicky".

@jvanasco
Copy link
Author

Calls intended for request.route_url('route-default_action') would almost always be made to an explicitly defined request.route_url('route')

I added a fix for the "//" issue, and also pushed the unit test which I forgot to commit.

This isn't a matter of saving 1 line of configuration code - this can save dozens of lines code , as this is a common pattern / need that is present in many frameworks.

The pylons implementation was indeed magicky in how someone would implement a route - however that was because it happened on a global scale and was altering the route to make something work by minimizing it. it was very easy to have unintended consequences.

This implementation is not magicky at all and is very straightforward - if you specify a default action for a route, it supports that default action to be used in place of an unspecified action. this behavior is essentially identical to specifying a "Directory Index" on a web/proxy/cache server, however it's within pyramid -- which means it is easier to test and more consolidated to maintain.

@mmerickel
Copy link
Member

So what am I missing here? Your patch simply calls add_handler twice. This saves the user from having to call it twice, but that's basically it AFAICT. This does not address the url generation issue.

With or without your patch, the following code is not valid in Pyramid:

config.add_handler('route', '/test/{action}')

request.route_url('route')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants