Skip to content

Apply right name to Route when created from methods - #1553

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

Kludex merged 13 commits into
Kludex:masterfrom
flxdot:method-route-naming

Conversation

@flxdot

@flxdot flxdot commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

Fixes #1552.

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

@Kludex

Kludex commented Mar 25, 2022

Copy link
Copy Markdown
Owner

Are there other cases we are forgetting?

@flxdot

flxdot commented Mar 25, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@Kludex

Kludex commented Mar 28, 2022

Copy link
Copy Markdown
Owner

Can we add a test for it?

Comment thread starlette/routing.py

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
Copy Markdown
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
Copy Markdown
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

flxdot commented Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Can we add a test for it?

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

@Kludex

Kludex commented Apr 8, 2022

Copy link
Copy Markdown
Owner

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

@flxdot

flxdot commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

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

Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
@Kludex

Kludex commented Apr 8, 2022

Copy link
Copy Markdown
Owner

Thanks for the tests!

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

@flxdot

flxdot commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

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

Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py
@Kludex

Kludex commented Apr 8, 2022

Copy link
Copy Markdown
Owner

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

Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
Comment thread tests/test_routing.py Outdated
@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

flxdot commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

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

@Kludex

Kludex commented Apr 8, 2022

Copy link
Copy Markdown
Owner

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