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

Apply right name to Route when created from methods #1553

Merged
merged 13 commits into from Apr 8, 2022
Merged

Apply right name to Route when created from methods #1553

merged 13 commits into from Apr 8, 2022

Conversation

flxdot
Copy link
Contributor

@flxdot flxdot commented Mar 25, 2022

Fixes #1552.

Adapted existing code to fix wrong name of a Route when set automatically for routes with methods as endpoints.

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 25, 2022

Are there other cases we are forgetting?

@flxdot
Copy link
Contributor Author

flxdot commented Mar 25, 2022

Are there other cases we are forgetting?

The only Callables I'm aware of are:

  • functions
  • classes implementing __call__
  • methods (incl. static & class methods)
  • lambda

But in the nature of the lambda function is anonymous, we can not read a name.

After consulting the docs of the inspect module again we could simplify the implementation by using inspect.routine:

def get_name(endpoint: typing.Callable) -> str:
    if inspect.isroutine(endpoint):
        return endpoint.__name__
    return endpoint.__class__.__name__

This might create issues with the lambda functions though:

from starlette.responses import JSONResponse
from starlette.routing import Route


async def my_function(request):
    return JSONResponse({'endpoint_type': 'function'})


class MyClass:
    def __call__(self, request):
        return JSONResponse({'endpoint_type': 'class'})


class MySpecialEndpointObject:
    async def my_method(self, request):
        return JSONResponse({'endpoint_type': 'method'})

    @classmethod
    async def my_classmethod(self, request):
        return JSONResponse({'endpoint_type': 'classmethod'})

    @staticmethod
    async def my_staticmethod(self, request):
        return JSONResponse({'endpoint_type': 'staticmethod'})


function_route = Route('/functionEndpoint', my_function)
class_route = Route('/classEndpoint', MyClass())
endpoint_obj = MySpecialEndpointObject()
method_route = Route('/methodEndpoint', endpoint_obj.my_method)
classmethod_route = Route('/classMethodEndpoint', endpoint_obj.my_classmethod)
staticmethod_route = Route('/staticMethodEndpoint', endpoint_obj.my_staticmethod)
lambda_route = Route('/lambdaEndpoint', lambda request: JSONResponse({'endpoint_type': 'lambda'}))

assert function_route.name == "my_function"
assert class_route.name == "MyClass"
assert method_route.name == "my_method"
assert classmethod_route.name == "my_classmethod"
assert staticmethod_route.name == "my_staticmethod"
assert lambda_route.name == "lambda" # AssertionError, because it will be: "<lambda>"

@adriangb
Copy link
Member

I think it's okay if lambdas aren't handled correctly

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 28, 2022

Can we add a test for it?

@@ -84,7 +84,7 @@ async def app(scope: Scope, receive: Receive, send: Send) -> None:


def get_name(endpoint: typing.Callable) -> str:
if inspect.isfunction(endpoint) or inspect.isclass(endpoint):
if inspect.isroutine(endpoint) or inspect.isclass(endpoint):
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Does the behavior of lambda changes now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No:

import inspect
my_lambda = lambda: 1
assert inspect.isfunction(my_lambda) == inspect.isroutine(my_lambda)

@flxdot
Copy link
Contributor Author

flxdot commented Mar 30, 2022

Can we add a test for it?

I'm currently quite busy. But I'll add tests next week.

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 8, 2022

@flxdot Are you be able to work on this soon or can I take it over?

@flxdot
Copy link
Contributor Author

flxdot commented Apr 8, 2022

@flxdot Are you be able to work on this soon or can I take it over?

Friday afternoons seem to be only free time atm.

Since get_name() is rather an internal function I decided not to test the function, but to rather test the behaviour of the common public usage (building of routes). If you would rather like to test the function I can quickly change it.

tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
@Kludex
Copy link
Sponsor Member

Kludex commented Apr 8, 2022

Thanks for the tests!

Yeah, no need to test specifically get_name. I've added some comments to simplify the tests added, jfyk.

@flxdot
Copy link
Contributor Author

flxdot commented Apr 8, 2022

Good call reducing the clutter in the test. Thanks for the hint!

tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
@Kludex
Copy link
Sponsor Member

Kludex commented Apr 8, 2022

Once the linter issues are resolved, we can merge it.

tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
tests/test_routing.py Outdated Show resolved Hide resolved
@Kludex Kludex changed the title add support to read route names from methods Apply right name to Route when created from methods Apr 8, 2022
@flxdot
Copy link
Contributor Author

flxdot commented Apr 8, 2022

sorry for all that back and forth. Should have dug deeper into the GitHub action to see, what is run.

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 8, 2022

Thanks @flxdot ! :)

@Kludex Kludex merged commit d7cbe2a into encode:master Apr 8, 2022
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.

Route naming introspection always return "method" for method endpoints
3 participants