search

zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
350 stars 356 forks source link

zulip: Give quicker feedback when credentials are wrong #627

Closed QEDK closed 3 years ago

QEDK commented 3 years ago

Fixes #533

QEDK commented 3 years ago

@zulipbot add "needs review"

QEDK commented 3 years ago

@showell @neiljp I'm not sure if I'm not using zulipbot right but I cannot add labels to mark this for review so I'm pinging both of you as both of you reviewed code in the other related pull request.

andersk commented 3 years ago

This causes many errors to be logged and then ignored (with sys.exit(), return 0, or falling off the end of an except block), and it doesn’t change the Client constructor at all. I’m pretty sure this is not what the author of #533 had in mind.

QEDK commented 3 years ago

This causes many errors to be logged and then ignored (with sys.exit(), return 0, or falling off the end of an except block), and it doesn’t change the Client constructor at all. I’m pretty sure this is not what the author of #533 had in mind.

This is modeled after #592 which is mostly what they wanted in #533 (per comments on that pull). I fixed the structuring, typos and rebasing.

andersk commented 3 years ago

592 has the same problems. One of them was already mentioned there, and I’ve commented there about the other.

QEDK commented 3 years ago

592 has the same problems. One of them was already mentioned there, and I’ve commented there about the other.

I'll open a new PR to address it in the other way.

showell commented 3 years ago

@QEDK I am going to close this, since you are going to work on a simpler approach. I hate to repeat what's already been said, but just to be completely clear, we want this change to be minimal code that always happens as part of the API automatically, ideally in the Client constructor or somewhere close. (I'm the original author of #533, so I just want to make it clear that everybody here is on the same page.)

Thanks for the initial effort! Looking forward to the next PR.