Skip to content

Allow for a view_selector of any length. #99

Closed
wants to merge 4 commits into from

3 participants

@garyvdm
garyvdm commented Jan 13, 2011

I would like to be able to change the view selector in my app from '@@' to '+'. Someone showed me how to change the view selector by subclassing ResourceTreeTraverser [1]. But this only allows for a view selector of a length of 2.

This small patch allows for one to use the above mentioned method to use a view selector of any length.

[1] http://stackoverflow.com/questions/4427617/change-the-view-prefix-in-pyramid-traversal-from-to

@mcdonc
Pylons Project member
mcdonc commented Jan 21, 2011

Sorry Gary, I hate to reject this patch, but the current traversal algorithm is the way it is for performance reasons and incurring a startswith and a len for this feature is not worth it for 99% of people. Thankfully you can replace the entire traverser (as you probably know already): http://docs.pylonsproject.org/projects/pyramid/dev/narr/hooks.html#changing-the-traverser . I'd suggest doing that for this feature.

@andreypopp

I think it's worth to revert to previous commit and still merge this pull request — performance penalty from len(string) is negligible — every implementation can get it in constant time, cause it's stored along with string.

Gary van der... added some commits Feb 4, 2011
@garyvdm
garyvdm commented Feb 4, 2011

I was a bit skeptical about how much this would hurt performance, so I did some experiments with timeit:

import timeit

setup="""
view_selector = "@@"
yes = "@@yes"
no = "no"
"""
print timeit.timeit("""
yes[:2] == view_selector; yes[2:]
no[:2] == view_selector; no[2:]
""", setup)

print timeit.timeit("""
yes.startswith(view_selector); yes[len(view_selector):]
no.startswith(view_selector); no[len(view_selector):]
""", setup)

print timeit.timeit("""
view_len = len(view_selector)

yes[:view_len] == view_selector; yes[view_len:]
no[:view_len] == view_selector; no[view_len:]
""", setup)

3 timeit's. They test:

  • Current pyramid method.
  • Method used in my first pull request.
  • Get the length of the view_selector on each run.

Results:

0.94603395462
1.65856909752
0.966178894043

So using startswith is very slow, but cost of len is very low, so I've updated my patch to use this method.

Gary

@mcdonc
Pylons Project member
mcdonc commented Apr 11, 2011

I'm sorry, after all this time, I'm still -1 on this patch.

Beyond the speed issue, I've been burned in the past (in Zope, primarily) by introducing "knobs on knobs", and this patch provides a knob (the view selector composition) on a knob (the default traversal policy). Using this pattern obscures the purpose of the original knob and, over time, turns the default implementation into its own complicated framework, making it difficult for people to know when to replace the implementation or just turn a knob. Doing this played a part in Zope's downfall due to overcomplication, and I'm not prepared to go there again.

I accepted Andrey's original knob patch which made it configurable when I should not have, but I don't feel like we're obligated to continue down that path.

My apologies, I'm going to close this.

@mcdonc mcdonc closed this Apr 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.