Skip to content

Conversation

casperdcl
Copy link
Member

@casperdcl casperdcl commented Mar 16, 2018

tqdm/_tqdm.py Outdated
from warnings import warn
warn("tqdm:disabling monitor support"
" (monitor_interval = 0) due to:\n" + str(e),
RuntimeWarning)
Copy link
Member Author

Choose a reason for hiding this comment

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

@kalefranz you OK with this?

Copy link

@kalefranz kalefranz Mar 16, 2018

Choose a reason for hiding this comment

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

I'd make a custom warning, so that it can be specifically disabled without disabling all RuntimeWarnings.

class TqdmDisabledMonitorWarning(RuntimeWarning):
    """Warned when the tqdm update interval monitor is disabled because of system limitations."""

And then, in application code, I'd just import the warning, and add

warnings.simplefilter('ignore', TqdmDisabledMonitorWarning)

The alternative is forcing applications to disable all RuntimeWarning which I don't think is what you want as a library author. By inheriting from RuntimeWarning instead of just Warning though, you allow users that flexibility if they want it.

@casperdcl casperdcl added p0-bug-critical ☢ Exception rasing to-review 🔍 Awaiting final confirmation need-feedback 📢 We need your response (question) to-merge ↰ Imminent synchronisation ⇶ Multi-thread/processing labels Mar 16, 2018
@codecov-io
Copy link

codecov-io commented Mar 16, 2018

Codecov Report

Merging #523 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   99.24%   99.24%   +<.01%     
==========================================
  Files           7        8       +1     
  Lines         659      665       +6     
  Branches      117      117              
==========================================
+ Hits          654      660       +6     
  Misses          3        3              
  Partials        2        2

Copy link

@kalefranz kalefranz left a comment

Choose a reason for hiding this comment

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

LGTM

@casperdcl casperdcl merged commit be21c5b into master Mar 16, 2018
@casperdcl casperdcl deleted the devel branch March 27, 2018 18:12
@casperdcl casperdcl restored the devel branch March 27, 2018 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-feedback 📢 We need your response (question) p0-bug-critical ☢ Exception rasing synchronisation ⇶ Multi-thread/processing to-merge ↰ Imminent to-review 🔍 Awaiting final confirmation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError: can't start new thread
3 participants