[Refactor] Fix clippy::too_many_arguments and rework error page rendering #529
Conversation
|
Hi @svenstaro, I currently base my new PRs on some previous PRs to avoid huge merge conflicts when merging. What do you think of such workflow? I found this necessary because PRs are piling up. |
|
Is such code refactoring welcome? I hope it is not invasive! |
aliemjay
changed the title
aliemjay
changed the title
aliemjay
force-pushed
the
src-refactor
branch
2 times, most recently
from
d83e429
to
168b686
Compare
|
@svenstaro I hope this gets reviewed next |
|
Sure but let's get #500 in first. We're almost there! \o/ |
There was a problem hiding this comment.
I get a plain (unthemed) error page for invalid or missing auth here. Granted, this also was the previous behavior but how about we fix this in the same PR? The 404 error, for instance, is already themed. It would make sense for 401/403 to also be themed and it would be more consistent, too.
|
While testing this, I noticed #588 which you could address in this PR as well (or it could just be a follow-up). |
Is it even possible to get themed in invalid auth? I mean unless we embed css. |
This should be easy. I'll implement it here. |
I suppose the internal route to the media resources doesn't really need to be protected, no secrets will ever be kept there. The theme information is stored inside the browser itself so that should also be always available. |
|
The rest of it looks like a vast improvement to me! Probably there are some parts which could be golfed a little (like the middleware bit) but I wouldn't immediately know how to best golf it. |
|
|
||
| div.error { | ||
| p { (error_code.to_string()) } | ||
| @for error in error_description.lines() { | ||
| p { (error) } | ||
| } | ||
| @if display_back_link { | ||
| // WARN don't expose random route! | ||
| @if !conf.random_route.is_some() { |
aliemjay
force-pushed
the
src-refactor
branch
2 times, most recently
from
d716163
to
5a87886
Compare
|
I snuck in #542. Could you rebase on top of that? Sorry! |
... page rendering Too many arguments are moved around and many of them are already stored in MiniserveConfig. Many of these are used to render error pages. To fix this issue, it was necessary to rework error page rendering: 1. Implement `ResponseError` for `ContextualError` so that it can be returned from service handlers as is and will then be automatically logged to the console and converted into an error response. 2. At service handler level, all error responses are now rendered as plain text. 3. 'error_page_middleware' is now responsible for the rendering of the final error page from plain text reponses. Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
|
#588 and themed 401 page should be fixed |
|
Very cool! |
Too many arguments are moved around and many of them are already stored in
MiniserveConfig. Many of these are used to render error pages.To fix this issue, it was necessary to rework error page rendering:
Implement
ResponseErrorforContextualErrorso that it can bereturned from service handlers as is and will then be automatically
logged to the console and converted into an error response.
At service handler level, all error responses are now rendered as
plain text.
error_page_middlewareis now responsible for the rendering of thefinal error page from plain text responses.
Based on #513
Closes #588