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

New exception page. #1386

Closed

Conversation

alex-oleshkevich
Copy link
Member

This PR updates debugger UI.

Features:

  • code preview
  • local variables for snippet
  • request/session/headers/etc details
  • beautify UI
  • responsible

UI is influenced by the Phoenix Framework error page

localhost_7001_alex

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 27, 2021

Beautiful. 👍

@Kludex Kludex linked an issue Dec 28, 2021 that may be closed by this pull request
@Kludex Kludex added clean up Refinement to existing functionality refactor Refactor code labels Dec 28, 2021
@alex-oleshkevich
Copy link
Member Author

Here is a full page screenshot with all sections open.

fullpage

@alex-oleshkevich
Copy link
Member Author

This handler may raise an exception if one of locals is QuerystringParser.
The bug itself is here Kludex/python-multipart#29, seems fixed but no new version has been released for a long time.

Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

This is nice.

Comment on lines +428 to +435
if any(
[
"key" in key,
"token" in key,
"password" in key,
"secret" in key,
]
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if any(
[
"key" in key,
"token" in key,
"password" in key,
"secret" in key,
]
):
if any(k in key for k in ["key", "token", "password", "secret"]):

Is this less readable? What do you think?

"secret" in key,
]
):
return "*" * 8
Copy link
Member

Choose a reason for hiding this comment

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

I haven't really checked any other projects, but do you think it would be useful to mask a part of the value? For example masking first half of the string? Or at least mask the length of the value, as I think this might be confusing.

@tomchristie
Copy link
Member

This looks fantastic.
Having said that it's also an awful lot of extra code to pull into the core package and maintain.

Presumably you'd also be able to package this up as a third party package, and document how to install it as a custom exception handler, right? (Or are there any blockers there that I've not figured out?)

A good rule of thumb for proposals to Starlette should probably be "if it can be a third party package, it should be".

That's worked really well for REST framework, and it'd work really well for Starlette too - help make sure we can just keep it small and well-scoped, but still allow folks to build fantastic stuff on top of it.

@alex-oleshkevich
Copy link
Member Author

Yes, I have the same thought.

@alex-oleshkevich
Copy link
Member Author

Available as https://github.com/alex-oleshkevich/starception

@alex-oleshkevich alex-oleshkevich deleted the fancy-debugger branch August 10, 2022 09:56
@tomchristie
Copy link
Member

Fantastic stuff.
Want to issue a pull request linking to it from https://www.starlette.io/exceptions/ and https://www.starlette.io/third-party-packages/ ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include more information in DebugMiddleware.
4 participants