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.

Sign up for GitHub

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

Jump to bottom

Implement __repr__ for route classes #1864

Merged
merged 17 commits into from Sep 23, 2022
Merged

Implement __repr__ for route classes #1864

merged 17 commits into from Sep 23, 2022

Conversation

alex-oleshkevich
Copy link
Member

@alex-oleshkevich alex-oleshkevich commented Sep 22, 2022 •
edited

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

image

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 22, 2022

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

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

?

@alex-oleshkevich alex-oleshkevich changed the title Implement __repr__ for Route Implement __repr__ for route classes Sep 22, 2022
@alex-oleshkevich alex-oleshkevich marked this pull request as ready for review September 22, 2022 11:59
@alex-oleshkevich
Copy link
Member Author

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 Kludex added this to the Version 0.21.0 milestone Sep 22, 2022
Kludex
Kludex requested changes Sep 22, 2022
View reviewed changes
Copy link
Sponsor Member

@Kludex Kludex left a comment •
edited

Choose a reason for hiding this comment

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

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
Kludex reviewed Sep 22, 2022
View reviewed changes
starlette/routing.py Outdated
def __repr__(self) -> str:
return (
f"<{self.__class__.__name__}: "
f"path={self.path}, name={self.name or ''}, app={self.app}>"
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

👍

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}>"

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

@Kludex Kludex mentioned this pull request Sep 22, 2022
Merged
8 tasks
Kludex
Kludex reviewed Sep 22, 2022
View reviewed changes
Copy link
Sponsor Member

@Kludex Kludex left a comment •
edited

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
starlette/routing.py Outdated Show resolved Hide resolved
starlette/routing.py Outdated Show resolved Hide resolved
starlette/routing.py Outdated Show resolved Hide resolved
starlette/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
alex-oleshkevich and others added 10 commits September 22, 2022 19:32
@alex-oleshkevich @Kludex
Update starlette/routing.py
af81645 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update starlette/routing.py
ff8d3a9 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update starlette/routing.py
c8ce4eb 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update starlette/routing.py
d7a721f 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update tests/test_routing.py
d66d460 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update tests/test_routing.py
442a951 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update tests/test_routing.py
c29042a 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update tests/test_routing.py
77d764d 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich @Kludex
Update tests/test_routing.py
162abb1 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich
fix linting issues and tests
c16be82
Kludex
Kludex approved these changes Sep 22, 2022
View reviewed changes
Copy link
Sponsor Member

@Kludex Kludex 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 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 Outdated Show resolved Hide resolved
@alex-oleshkevich
Copy link
Member Author

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
Kludex previously approved these changes Sep 22, 2022
View reviewed changes
@Kludex
Copy link
Sponsor Member

Kludex commented Sep 22, 2022

image

I'm not able to merge it 😅

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 22, 2022

Does anyone know why I can't merge? 🤔

@alex-oleshkevich
Copy link
Member Author

@Kludex what's behind "statuses" link?

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 23, 2022

@Kludex what's behind "statuses" link?

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

@Kludex Kludex dismissed their stale review September 23, 2022 12:41

I'm not able to merge.

@Kludex Kludex marked this pull request as draft September 23, 2022 12:43
@Kludex Kludex marked this pull request as ready for review September 23, 2022 12:43
@Kludex
Copy link
Sponsor Member

Kludex commented Sep 23, 2022

I don't know 👍

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 23, 2022

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

image

@Kludex Kludex changed the base branch from master to security-policy September 23, 2022 12:52
@Kludex Kludex changed the base branch from security-policy to master September 23, 2022 12:52
@Kludex Kludex merged commit 70971ea into encode:master Sep 23, 2022
@Kludex
Copy link
Sponsor Member

Kludex commented Sep 23, 2022

Easy. 👀

aminalaee pushed a commit that referenced this pull request Feb 13, 2023
@alex-oleshkevich @Kludex @aminalaee
Implement __repr__ for route classes (#1864)
4f8286a 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kludex Kludex Kludex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 0.21.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alex-oleshkevich @Kludex