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

telegram: add delete() when leave=False #1189

Merged
merged 2 commits into from Jun 30, 2021

Conversation

raulsaavedr
Copy link
Contributor

Add an option for delete bar message with the deleteMessage method in Bot API.

Add an option for delete bar message with the deleteMessage method in Bot API.
@raulsaavedr
Copy link
Contributor Author

This would be the behaviour

telegram_delete_method_pull

@casperdcl
Copy link
Sponsor Member

Ideally this would be used when leave=False?

@casperdcl casperdcl self-assigned this Jun 20, 2021
@casperdcl casperdcl added p3-enhancement 🔥 Much new such feature submodule ⊂ Periphery/subclasses to-review 🔍 Awaiting final confirmation labels Jun 20, 2021
@casperdcl casperdcl added this to In Progress in Casper Jun 20, 2021
@casperdcl casperdcl added this to the Non-breaking milestone Jun 20, 2021
@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #1189 (c36a011) into master (c1ec3b1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1189   +/-   ##
=======================================
  Coverage   89.88%   89.88%           
=======================================
  Files          26       26           
  Lines        1710     1710           
  Branches      284      284           
=======================================
  Hits         1537     1537           
  Misses        128      128           
  Partials       45       45           

if self.disable:
return
super(tqdm_telegram, self).close()
if not (self.leave or (self.leave is None and self.pos == 0)):
Copy link
Sponsor Member

@casperdcl casperdcl Jun 20, 2021

Choose a reason for hiding this comment

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

borrowing usual leave=None|False|True logic here.

Potential future issue is if people want different behaviour for terminal & telegram outputs (leave one but not the other). They'd have to override the close() method. Don't think we need to explicitly support/worry about that case for now.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the leave variable is perfect, it fits our needs, and yes the default behaviour would be leave = False. In the meantime we don't worry about that case for terminal and telegram outputs. When the close() method is called just delete the message from telegram and stop the bar from terminal.

@casperdcl casperdcl changed the title Update telegram.py to add a delete method telegram: add delete() when leave=False Jun 20, 2021
- also misc minor tidy
@casperdcl casperdcl changed the base branch from master to devel June 30, 2021 16:57
@casperdcl casperdcl merged commit 4735e81 into tqdm:devel Jun 30, 2021
Casper automation moved this from In Progress to Done Jun 30, 2021
@casperdcl casperdcl added to-merge ↰ Imminent and removed to-review 🔍 Awaiting final confirmation labels Jun 30, 2021
@casperdcl casperdcl mentioned this pull request Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-enhancement 🔥 Much new such feature submodule ⊂ Periphery/subclasses to-merge ↰ Imminent
Projects
Casper
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants