Skip to content

Apply right name to Route when created from methods#1553

Merged
Kludex merged 13 commits intoKludex:masterfrom
flxdot:method-route-naming
Apr 8, 2022
Merged

Apply right name to Route when created from methods#1553
Kludex merged 13 commits intoKludex:masterfrom
flxdot:method-route-naming

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
Owner

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
Contributor

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

@Kludex
Copy link
Owner

Kludex commented Mar 28, 2022

Can we add a test for it?


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
Owner

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
Owner

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.

@Kludex
Copy link
Owner

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!

@Kludex
Copy link
Owner

Kludex commented Apr 8, 2022

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

@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
Owner

Kludex commented Apr 8, 2022

Thanks @flxdot ! :)

@Kludex Kludex merged commit d7cbe2a into Kludex: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