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

Add Conflict error (HTTP error code 409) #1154

Merged
merged 4 commits into from Oct 29, 2018

Conversation

@jsmnbom
Copy link
Member

jsmnbom commented Jul 8, 2018

Fixes #1143.

Do we need tests for stuff like this?

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Jul 11, 2018

hmm, Not to be blunt but why should we patch a user_error? I think the case in #1143 is too specific to add this feature..

@jsmnbom

This comment has been minimized.

Copy link
Member Author

jsmnbom commented Jul 22, 2018

The way I see it, is that telegram is returning a custom http error code, which we should handle on par with how we handle other 4xx error codes. Adding the conflicting id is just a quality of life type of improvement in my opinion. Doesn't really harm anything, but will probably make it easier to debug issues for some people.


class Conflict(TelegramError):
def __init__(self, msg, url):
match = re.search(r'bot(\d+):.*/', url)

This comment has been minimized.

Copy link
@tsnoam

tsnoam Sep 8, 2018

Member

Can you please add a comment on the code itself what is the input string and why it means a conflicting bot id? (in favour of future us who won't remember all the details...)

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Oct 2, 2018

What's the status on this? We discussed it a few times. I seem to remember we decided not to add this except for the exception?

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 2, 2018

What's the status on this? We discussed it a few times. I seem to remember we decided not to add this except for the exception?

Yes, exactly. @jsmnbom was on it. I thought it was already merged.

@jsmnbom

This comment has been minimized.

Copy link
Member Author

jsmnbom commented Oct 2, 2018

Yes, I'm on it, will update the PR tomorrow when I have time :)

@jsmnbom jsmnbom changed the title Add conflicting bot id to conflict error message. Add Conflict error (HTTP error code 409) Oct 29, 2018
@jsmnbom jsmnbom dismissed tsnoam’s stale review Oct 29, 2018

Outdated + discussed in internal chat.

@jsmnbom jsmnbom merged commit c9630ee into master Oct 29, 2018
5 checks passed
5 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 92.55%)
Details
codecov/project 92.61% (+0.06%) compared to f2b0672
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsmnbom jsmnbom deleted the fix-1143 branch Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.