Nicer formatting of fallback, allow HTML code to be changed from 500#129
Nicer formatting of fallback, allow HTML code to be changed from 500#129
Conversation
Barecheck - Code coverage reportTotal: 89.19%Your code coverage diff: 0.01% ▴ Uncovered files and lines
|
JohemianKnapsody
changed the title
|
I think the improved format is great. I am less convinced by the code: I think this might be better handled by updating Connect to open a page to display 5xx error messages. In the long term we should consider changing the way Connect tests for microscopes so it doesn't rely on the v1 API (unless @julianstirling did that already), and perhaps adding an explicit test for LabThings fallback servers so they can show up as broken microscopes. I assume the dead microscope only shows up if you enter the address manually? |
|
I was going to agree it was the wrong code until I read this
It seems like it was invented exactly for this use where you want to notify there is an error but want the client to act like everything is fine and display something. I thing re-defining how electron handles 5xx errors seems like a can of worms, and if Apache felt the need for this code to exist we can't be the only ones. Somewhat I think 5xx is the wrong error anyway, because while there is an internal server error, it isn't like it is a full 500 error which means that the server was unable to give you a helpful response. We have a helpful response with HTML content, it is just unfortunately helpful response that things are broken. |
|
I personally would merge this now. But I won't while @rwb27 has reservations. One option would be to make setting the code an option so that 500 can be the default given by labthings, but instead downstream can customise. All that would be needed is (I can't suggest changes to unchanged lines in GitHub): add to line 15: self.html_code = 500as 74 would change to return HTMLResponse(content=content, status_code=app.html_code)This way it becomes an OpenFlexure question of if 218 makes sense. |
|
I have added the change to make the code stable after discussing with Joe. This way LabThings doesn't have to be opinionated about the error code. |
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
I think this is ready to go.
I have made the HTML code 500 by standard, but configurable and moved the <style> block into the <head> block.
julianstirling
force-pushed
the
reformat-fallback
branch
from
cf73dda to
c0a3eb6
Compare
julianstirling
changed the title
|
I don't have a strong feeling for Julian's suggestion to move the |
3 participants
Style changes:
<pre>tag, preventing horizontal scroll barsChanging status code to
206 (partial success)reading about 206, it's partial success if the client only requests partial, which doesn't apply hereCode to 218 (this is fine) until someone finds a better one
Reason is that Connect won't open a tab returning a 5xx message, so we can't see the fallback messages in Connect. This will also require getting rid of Connect's version checking that we're connecting with a v2 microscope...
Before
After