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

Raise exceptions for {"result": "error"} in zulip apis. #672

Open
LoopThrough-i-j opened this issue Apr 2, 2021 · 10 comments
Open

Raise exceptions for {"result": "error"} in zulip apis. #672

LoopThrough-i-j opened this issue Apr 2, 2021 · 10 comments

Comments

@LoopThrough-i-j
Copy link
Contributor

LoopThrough-i-j commented Apr 2, 2021

Currently, zulip API doesn't handle {"result": "error"} and sends send back whatever comes from the server. We would want to handle these errors by raising exceptions in the API layer whenever there is a {"result": "error"}.

Reference

@alexzw7
Copy link
Collaborator

alexzw7 commented Apr 7, 2021

@zulipbot claim

@zulipbot
Copy link
Member

zulipbot commented Apr 7, 2021

Hello @alexzw7!

Thanks for your interest in Zulip! You have attempted to claim an issue without the labels "help wanted", "good first issue". Since you're a new contributor, you can only claim and submit pull requests for issues with the help wanted or good first issue labels.

If this is your first time here, we recommend reading our guide for new contributors before getting started.

@alexzw7
Copy link
Collaborator

alexzw7 commented Apr 7, 2021

@zulipbot add "good first issue"

@alexzw7
Copy link
Collaborator

alexzw7 commented Apr 7, 2021

@zulipbot claim

@zulipbot
Copy link
Member

zulipbot commented Apr 7, 2021

Welcome to Zulip, @alexzw7! We just sent you an invite to collaborate on this repository at https://github.com/zulip/python-zulip-api/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

@alexzw7 alexzw7 removed their assignment Apr 11, 2021
@LoopThrough-i-j
Copy link
Contributor Author

@zulipbot remove "good first issue"

@PIG208
Copy link
Member

PIG208 commented May 9, 2021

A brief note for this one here: we might need to add a common exception class called BotError and specify a set of sub errors like AccessDenied etc. (we need to discuss the naming). I think we can have a protocol that can be shared with embedded bots as well.
An anticipated result of this might be that the user will no longer need to examine the returned dictionary from the API and instead they can simply do error handling with try-except statements.

@timabbott
Copy link
Member

Yeah, I think that's a good idea.

@neiljp
Copy link
Contributor

neiljp commented May 28, 2021

Is it the intent of this issue to evolve all of the zulip python API to have different return values and raise exceptions instead?
While ZT can adapt to this per zulip library release, this seems like a big API transition to make for all dependent packages/scripts.

If this is an evolution of zulip, then BotError seems misnamed, unless this issue is specific to the bots library instead?

@timabbott
Copy link
Member

I think so. One theory is that we could provide a legacy_exception_handling flag to the Client() constructor that resulted in it using the old error-handling method of converting errors for backwards-compatibility.

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

No branches or pull requests

6 participants