Feature/improve lettera error messages #1486
Conversation
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
| @@ -633,3 +639,105 @@ editorialIdToUuid editorialId = | |||
| url_namespace :: UUID.UUID | |||
| url_namespace = unsafePartial $ fromJust $ UUID.parseUUID "6ba7b811-9dad-11d1-80b4-00c04fd430c8" | |||
|
|
|||
| encodeRequestHeaders :: Array AX.RequestHeader -> Maybe Json | |||
There was a problem hiding this comment.
I think Lettera.Models is too busy already and nothing in here uses these so it's not justified by dependencies either. The focus of this module is diluted if it gets Affjax imports to it. If you're moving these from Lettera make it a new module. Lettera.Requests or if you have a better idea.
There was a problem hiding this comment.
New module sounds like a great idea!
|
|
||
| data LetteraError | ||
| = ResponseError | ||
| { letteraErrorRequest :: Json |
There was a problem hiding this comment.
I think I said it already but don't use Json on this level. Leave generating it to the encodeJson instance. Ditto for HttpError. If we need users of this code to do actions based on responses then that's a lot more convenient format.
There was a problem hiding this comment.
Okay, but then we'll have more polymorphic type variables to deal with.
There was a problem hiding this comment.
I started to think that maybe including requests and responses in the error type is not the best solution to start with. That information is needed for logging but is it really an essential part of an error value? Perhaps we could consider moving the logging from Mosaico to Affresco to the place where we have the best information about the requests, responses and/or other context?
There was a problem hiding this comment.
It seems that If I generalise the Json type occurrences into type variables, then Mosaico and Affresco disagree whether the getFrontpage and getFrontpageHtml are dealing with LetteraResponse String String a or LetteraResponse Json Json a.
| encodeRequestHeaders [] = Nothing | ||
| encodeRequestHeaders hs = | ||
| let encodeHeader h obj = flip extend obj $ case h of | ||
| AX.Accept (MediaType v) -> "Accept" := v |
There was a problem hiding this comment.
If you're doing white spaces for alignment use it on := too.
| AX.Accept (MediaType v) -> "Accept" := v | ||
| AX.ContentType (MediaType v) -> "ContentType" := v | ||
| AX.RequestHeader k v -> k := v | ||
| in Just $ foldr encodeHeader jsonEmptyObject hs |
There was a problem hiding this comment.
Is this going to include authorization headers in it? I think a better default would be to scrub them from LetteraResponse (and if convenient make it easy in the source to enable them for dev use when needed).
There was a problem hiding this comment.
Good point! I'll have a look at this tomorrow.
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
|
Deploy previews are ready 😎 |
jllang
force-pushed
the
feature/improve-lettera-error-messages
branch
from
ef4c146
to
74ec015
Compare
|
Deploy previews are ready 😎 |
No description provided.