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

Add "LIFT" sentinel for context and name arguments to add_view #3739

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ericatkin
Copy link
Contributor

@ericatkin ericatkin commented Dec 23, 2023

This change adds a "LIFT" sentinel that signals context and name arguments to add_view should be computed at scan time. This is useful when using venusian.lift to propagate view_config decorators through a view class inheritance hierarchy. LIFT for name in conjunction with view_defaults helps reduces boilerplate and for context it is essential to avoid pyramid.exceptions.ConfigurationConflictError.

I've included an integration test below, but I need help making this into unit tests. Tox is failing for coverage but there is no other breakage.

from pyramid.httpexceptions import HTTPOk
from pyramid.config import Configurator
from pyramid.view import LIFT, view_config, view_defaults
from venusian import lift
from wsgiref.simple_server import make_server


@view_defaults(context=LIFT, name=LIFT)
class A:
    resources = {}

    def __init_subclass__(cls):
        A.resources[cls.__name__] = cls
        
    def __init__(self, request):
        self.request = request
    
    def __getitem__(self, key):
        return self.resources[key](self.request)
    
    @view_config()
    def foo(self):
        return HTTPOk(body=f'{self.__class__.__name__}.foo')

    @view_config()
    def bar(self):
        return HTTPOk(body=f'{self.__class__.__name__}.bar')


A.resources['A'] = A


@lift()
@view_defaults(context=LIFT, name=LIFT)
class B(A):
    @view_config()
    def baz(self):
        return HTTPOk(body=f'{self.__class__.__name__}.baz')


@lift()
@view_defaults(context=LIFT, name=LIFT)
class C(B):
    @view_config()
    def burp(self):
        return HTTPOk(body=f'{self.__class__.__name__}.burp')


    @view_config(name='hiccup')
    def spasm(self):
        return HTTPOk(body=f'{self.__class__.__name__}.spasm')


if __name__ == '__main__':
    with Configurator(root_factory=A) as config:
        config.scan()
        app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 6543, app)
    server.serve_forever()

'''
(
URL=localhost:6543/foo; test `curl -s $URL` = 'A.foo' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/bar; test `curl -s $URL` = 'A.bar' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/baz; curl -s -f $URL > /dev/null && echo FAIL $URL || echo PASS $URL
URL=localhost:6543/B/foo; test `curl -s $URL` = 'B.foo' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/bar; test `curl -s $URL` = 'B.bar' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/baz; test `curl -s $URL` = 'B.baz' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/burp; curl -s -f $URL > /dev/null && echo FAIL $URL || echo PASS $URL
URL=localhost:6543/B/C/foo; test `curl -s $URL` = 'C.foo' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/C/bar; test `curl -s $URL` = 'C.bar' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/C/baz; test `curl -s $URL` = 'C.baz' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/C/burp; test `curl -s $URL` = 'C.burp' && echo PASS $URL || echo FAIL $URL
URL=localhost:6543/B/C/hiccup; test `curl -s $URL` = 'C.spasm' && echo PASS $URL || echo FAIL $URL
)
'''

Copy link

@barbiek1st barbiek1st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barbiek001st

src/pyramid/config/views.py Outdated Show resolved Hide resolved
@mmerickel
Copy link
Member

can you please target the main branch with this initial PR? once that's merged we'll backport it to the 2.0-branch potentially.

@ericatkin ericatkin changed the base branch from 2.0-branch to main January 3, 2024 17:35
Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against this PR at all but I literally cannot read this code and understand it. I've never used venusian's lift / subclassing. So if it's waiting for me I think I'm going to need some handholding.

It appears to be using traversal with the class being used as both the view and the resource. With the view name being the method name due to that being the default behavior of how traversal works.

src/pyramid/view.py Outdated Show resolved Hide resolved
src/pyramid/view.py Show resolved Hide resolved
@mmerickel
Copy link
Member

Can you paste a version of this code that works without this feature so I can understand it better?

@mmerickel
Copy link
Member

tldr seems to be:

  • context=LIFT is magic to say "use this class itself as context"
  • view=LIFT is magic to say "use the name of the method as the name of the view"

Yes? No?

@mmerickel
Copy link
Member

If the TLDR is correct it's kind of weird to hide that behavior behind the term "LIFT" as it doesn't seem to really match what lift means in venusian. But I'm learning as I think about this.

Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests / coverage for this feature.

@ericatkin
Copy link
Contributor Author

tldr seems to be:

  • context=LIFT is magic to say "use this class itself as context"
  • view=LIFT is magic to say "use the name of the method as the name of the view"

Yes? No?

Yes, but we need the magic because the class isn't defined until the venusian scan is happening. The context= is applied via view_defaults prior to any subclass that lift is operating on. The name= (you said view=, assumed typo) isn't strictly needed, but reduces the redundancy of repeating the def name for view_config which is very common.

@mmerickel
Copy link
Member

mmerickel commented Feb 1, 2024

None of this is specific to the lift feature though is it?

name=LIFT could just as easily be used on a normal view:

@view_config(name=VALUE_FROM_FUNCTION_NAME)
def foo(request):
    ...

and similarly context=LIFT could just as easily be:

class FooResource:
    def __init__(self, request):
        self.request = request

    @view_config(context=SELF)
    def foo(self):
        ...

I don't think name is really buying us much here and think it should be dropped. I don't really see the benefit considering Pyramid has gone for so long without a way to do this, requiring an explicit name argument.

For context, doing something here is interesting because it's not possible in my example above to use context=FooResource as the class isn't defined yet, so there's a chicken-and-egg issue there similar to python typing using -> Self as a valid return type. https://peps.python.org/pep-0673/

What if we supported typing.Self for this case?

@ericatkin
Copy link
Contributor Author

ericatkin commented Feb 1, 2024

None of this is specific to the lift feature though is it?

name=LIFT could just as easily be used on a normal view:

@view_config(name=VALUE_FROM_FUNCTION_NAME)
def foo(request):
    ...

and similarly context=LIFT could just as easily be:

class FooResource:
    def __init__(self, request):
        self.request = request

    @view_config(context=SELF)
    def foo(self):
        ...

I don't think name is really buying us much here and think it should be dropped. I don't really see the benefit considering Pyramid has gone for so long without a way to do this, requiring an explicit name argument.

For context, doing something here is interesting because it's not possible in my example above to use context=FooResource as the class isn't defined yet, so there's a chicken-and-egg issue there similar to python typing using -> Self as a valid return type. https://peps.python.org/pep-0673/

What if we supported typing.Self for this case?

You're right. Not specific to LIFT. That is just how I'll use it and I needed a name. Names are sometimes hard! :)

I like typing.Self. I'll work on a commit for that soon. BTW, I'm trying to get some tests written, but having a hard time. I've been trying to call add_view and just verify that the sentinel was replaced in the view registry, but I don't understand those internals well and haven't yet had success.

Re: name. it could be used on a normal view, but that would be pointless. The value is in using with view_defaults, hence the error if not used on a class method. Now, imagine a class with many dozens of view methods. It is tedious and redundant to have to add the name argument to all the view_config decorators. This is precisely what view_defaults is for IMO. For myself, this functionality is worth the cost, which is a test and some documentation as the implementation is hardly 2 lines and is optional for all those who don't care, but it's not a hill I'll die on. The context sentinel is essential.

I'll get to these changes asap, but it's an after hours project for me :)
Thanks.

@ericatkin ericatkin requested a review from mmerickel June 24, 2024 21:54
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.

3 participants