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

contrib.telegram: gracefully handle creation rate limit #1223

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

@casperdcl casperdcl added p0-bug-critical ☢ Exception rasing submodule ⊂ Periphery/subclasses to-merge ↰ Imminent c1-quick 🕐 Complexity low labels Aug 4, 2021
@casperdcl casperdcl added this to the Non-breaking milestone Aug 4, 2021
@casperdcl casperdcl self-assigned this Aug 4, 2021
@casperdcl casperdcl mentioned this pull request Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #1223 (e02e77a) into devel (0b64cfe) will increase coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #1223      +/-   ##
==========================================
+ Coverage   89.59%   89.88%   +0.29%     
==========================================
  Files          26       26              
  Lines        1721     1721              
  Branches      286      286              
==========================================
+ Hits         1542     1547       +5     
+ Misses        132      128       -4     
+ Partials       47       46       -1     

@casperdcl casperdcl merged commit 29ff37d into devel Aug 4, 2021
@casperdcl casperdcl deleted the telegram-rate-limit branch August 4, 2021 12:29
Copy link

@Hagai Hagai left a comment

Choose a reason for hiding this comment

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

I love the improvements in this merge, I suggested robustness and readability improvements.

warn("Creation rate limit: try increasing `mininterval`.",
TqdmWarning, stacklevel=2)
else:
self._message_id = res['result']['message_id']
Copy link

Choose a reason for hiding this comment

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

Since this result came from external API I would assert 'result' and 'message_id' exists before accessing them.
Example for a possible error: authentication failure.

self.text = self.__class__.__name__
self.message_id
Copy link

Choose a reason for hiding this comment

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

I guess you intended to initialize '_message_id' property using this line, while it works, I think it can be clearer.
I would suggest a minor modification to avoid Codacy flag this as "Statement seems to have no effect" and "Access to member '_message_id' before its definition line 56".
Here define "self._message_id = None" and later instead of "hasattr(self, '_message_id')" use "self._message_id is None".
I would also put lines 44 to 56 in separate method (named initialize_message_id or a similar name). And I would explicitly call it to make the intension clearer (instead of accessing the message_id preperty).
message_id property will also call this method if self._message_id is None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c1-quick 🕐 Complexity low p0-bug-critical ☢ Exception rasing submodule ⊂ Periphery/subclasses to-merge ↰ Imminent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants