Implement repr for route classes #1864

alex-oleshkevich · 2022-09-22T11:48:53Z

This little quality of life improvement makes debugging a bit easier by providing route information in the debugger:

Kludex · 2022-09-22T11:55:51Z

Are you strong about the style in this PR or can we do:

Route(path="/login", name="login", methods={"GET", "POST"})

?

alex-oleshkevich · 2022-09-22T12:07:33Z

Are you strong about the style in this PR or can we do:
Route(path="/login", name="login", methods={"GET", "POST"})
?

No, that's a matter of taste. If you think the proposed solution is more readable, I am fine to change.

Kludex

My previous comment is not an opinion anymore, as I've noticed that we are already using the style I mentioned in our source code. Would you mind adapting it @alex-oleshkevich ?

Kludex · 2022-09-22T15:07:19Z

starlette/routing.py

+    def __repr__(self) -> str:
+        return (
+            f"<{self.__class__.__name__}: "
+            f"path={self.path}, name={self.name or ''}, app={self.app}>"


👍

Suggested change

f"path={self.path}, name={self.name or ''}, app={self.app}>"

f"path={self.path}, name={self.name or ''}, app={self.app!r}>"

Actually, the !r is used on all parameters in the other __repr__ that we have in code 🤔

Ah, I got why... It's because with them, there's no need to wrap on parenthesis!

Kludex

The comments are mainly to comply with the style we have on the other __repr__. 🙏

I think the suggestions on the tests are correct, but not sure. 👀

starlette/routing.py

tests/test_routing.py

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Kludex

I'm not sure about the Router.__repr__, as I don't see much value there, but I'll let you judge that.

Let me know your judgement, and I'll merge it. 👍

Thanks for making an effort to improve our users' lives @alex-oleshkevich 🙏

starlette/routing.py

alex-oleshkevich · 2022-09-22T16:57:26Z

I'm not sure about the Router.__repr__, as I don't see much value there, but I'll let you judge that.

Let me know your judgement, and I'll merge it. +1

Thanks for making an effort to improve our users' lives @alex-oleshkevich pray

okay, i will remove it. this is too confusing.

Kludex · 2022-09-22T17:21:19Z

I'm not able to merge it 😅

Kludex · 2022-09-22T18:54:43Z

Does anyone know why I can't merge? 🤔

alex-oleshkevich · 2022-09-22T22:07:44Z

@Kludex what's behind "statuses" link?

Kludex · 2022-09-23T12:40:21Z

@Kludex what's behind "statuses" link?

https://docs.github.com/rest/commits/statuses

I'm not able to merge.

Kludex · 2022-09-23T12:44:49Z

I don't know 👍

Kludex · 2022-09-23T12:52:06Z

The target is wrong... It should be "master", but it's "encode:master"... I don't know what happened here.

Kludex · 2022-09-23T12:55:03Z

Easy. 👀

* implement __repr__ for Route * implemenr __repr__ for WebSocketRoute, Host, and Mount. * fix linting issues * change repr format * force repr() for inner apps * Update starlette/routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update starlette/routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update starlette/routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update starlette/routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update tests/test_routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update tests/test_routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update tests/test_routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update tests/test_routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * Update tests/test_routing.py Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com> * fix linting issues and tests * remove repr from Router Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

alex-oleshkevich added 3 commits September 22, 2022 14:35

implement __repr__ for Route

0ac936e

implemenr __repr__ for WebSocketRoute, Host, and Mount.

049edb9

fix linting issues

8768bb9

alex-oleshkevich changed the title ~~Implement __repr__ for Route~~ Implement __repr__ for route classes Sep 22, 2022

alex-oleshkevich marked this pull request as ready for review September 22, 2022 11:59

Kludex added this to the Version 0.21.0 milestone Sep 22, 2022

Kludex requested changes Sep 22, 2022

View reviewed changes

Kludex reviewed Sep 22, 2022

View reviewed changes

Kludex mentioned this pull request Sep 22, 2022

Version 0.21.0 #1812

Merged

8 tasks

alex-oleshkevich added 2 commits September 22, 2022 18:15

change repr format

c985166

force repr() for inner apps

94df496

alex-oleshkevich requested a review from Kludex September 22, 2022 15:27

Kludex reviewed Sep 22, 2022

View reviewed changes

alex-oleshkevich and others added 10 commits September 22, 2022 19:32

Update starlette/routing.py

af81645

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update starlette/routing.py

ff8d3a9

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update starlette/routing.py

c8ce4eb

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update starlette/routing.py

d7a721f

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update tests/test_routing.py

d66d460

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update tests/test_routing.py

442a951

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update tests/test_routing.py

c29042a

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update tests/test_routing.py

77d764d

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update tests/test_routing.py

162abb1

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

fix linting issues and tests

c16be82

Kludex approved these changes Sep 22, 2022

View reviewed changes

starlette/routing.py Outdated Show resolved Hide resolved

remove repr from Router

8087806

Kludex previously approved these changes Sep 22, 2022

View reviewed changes

Merge branch 'master' into route-repr

c81002e

Kludex approved these changes Sep 23, 2022

View reviewed changes

Kludex marked this pull request as draft September 23, 2022 12:43

Kludex marked this pull request as ready for review September 23, 2022 12:43

Kludex changed the base branch from master to security-policy September 23, 2022 12:52

Kludex changed the base branch from security-policy to master September 23, 2022 12:52

Kludex merged commit 70971ea into encode:master Sep 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement repr for route classes #1864

Implement repr for route classes #1864

alex-oleshkevich commented Sep 22, 2022 •

edited

Loading

Kludex commented Sep 22, 2022

alex-oleshkevich commented Sep 22, 2022

Kludex left a comment •

edited

Loading

Kludex Sep 22, 2022

Kludex Sep 22, 2022

Kludex Sep 22, 2022

Kludex left a comment •

edited

Loading

Kludex left a comment

alex-oleshkevich commented Sep 22, 2022

Kludex commented Sep 22, 2022

Kludex commented Sep 22, 2022

alex-oleshkevich commented Sep 22, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

	f"path={self.path}, name={self.name or ''}, app={self.app}>"
	f"path={self.path}, name={self.name or ''}, app={self.app!r}>"

Implement __repr__ for route classes #1864

Implement __repr__ for route classes #1864

Conversation

alex-oleshkevich commented Sep 22, 2022 • edited Loading

Kludex commented Sep 22, 2022

alex-oleshkevich commented Sep 22, 2022

Kludex left a comment • edited Loading

Choose a reason for hiding this comment

Kludex Sep 22, 2022

Choose a reason for hiding this comment

Kludex Sep 22, 2022

Choose a reason for hiding this comment

Kludex Sep 22, 2022

Choose a reason for hiding this comment

Kludex left a comment • edited Loading

Choose a reason for hiding this comment

Kludex left a comment

Choose a reason for hiding this comment

alex-oleshkevich commented Sep 22, 2022

Kludex commented Sep 22, 2022

Kludex commented Sep 22, 2022

alex-oleshkevich commented Sep 22, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

Kludex commented Sep 23, 2022

Implement repr for route classes #1864

Implement repr for route classes #1864

alex-oleshkevich commented Sep 22, 2022 •

edited

Loading

Kludex left a comment •

edited

Loading

Kludex left a comment •

edited

Loading