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

Fix local error messages #865

Closed
wants to merge 1 commit into from
Closed

Fix local error messages #865

wants to merge 1 commit into from

Conversation

iMicknl
Copy link
Owner

@iMicknl iMicknl commented May 28, 2023

Follow up on #843 (comment).

@shypike can you verify if this is the right fix?

@shypike
Copy link
Contributor

shypike commented May 29, 2023

It should be OK, but I'll add it to my own installation. And I will try to catch the error condition.
I can give you feedback tomorrow.

@iMicknl
Copy link
Owner Author

iMicknl commented May 29, 2023

It should be OK, but I'll add it to my own installation. And I will try to catch the error condition. I can give you feedback tomorrow.

Thanks! Let me know :) would be good to catch some JSON files so we can eventually add tests.

@@ -809,7 +809,7 @@ async def check_response(response: ClientResponse) -> None:
) from error

if result.get("errorCode"):
message = result.get("error").strip("'")
message = result.get("error").strip('"')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we don’t add any test, it would be nice to add as comment an example of why we trip the ".

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agree! Preferably both 😄.

@shypike
Copy link
Contributor

shypike commented May 31, 2023

It looks like this one is the only deviation:

{"error":"\"No registered event listener.\"","errorCode":"UNSPECIFIED_ERROR"}

@shypike
Copy link
Contributor

shypike commented May 31, 2023

I've tested some issues with both Cloud and Local (I cannot reproduce them all).
Cloud says:

{"errorCode":"UNSPECIFIED_ERROR","error":"No registered event listener"}
{"errorCode":"RESOURCE_ACCESS_DENIED","error":"Not authenticated"}

Local says:

400: {"error":"\"No registered event listener.\"","errorCode":"UNSPECIFIED_ERROR"}
401: {"error":"Not authenticated.","errorCode":"RESOURCE_ACCESS_DENIED"}
400: {"error":"Unknown object.","errorCode":"UNSPECIFIED_ERROR"}
401: {"error":"Missing authorization token.","errorCode":"RESOURCE_ACCESS_DENIED"}

Note also that Local ends some messages with a full-stop "." and Cloud does not.
It's these nasty little differences that make me prefer in instead of ==.
Or at least use this:

message = result.get("error").strip("\"'.")

@iMicknl
Copy link
Owner Author

iMicknl commented Jun 2, 2023

Thanks @shypike. Do you know which HTTP code these requests have?

@shypike
Copy link
Contributor

shypike commented Jun 3, 2023

I've added the error codes to the original message,
Of course the required change is not just stripping with .strip(""'.") but also removing the trailing "." from the string comparisons.

@iMicknl
Copy link
Owner Author

iMicknl commented Jun 4, 2023

Thanks! @shypike, I implemented it in #870.

@iMicknl iMicknl closed this Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants