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

Jump to bottom

added logging sub-module #1155

Merged
merged 2 commits into from Apr 5, 2021
Merged

added logging sub-module #1155

merged 2 commits into from Apr 5, 2021

Conversation

de-code
Copy link
Contributor

@de-code de-code commented Apr 1, 2021 •
edited

this implements a slightly improved version of #786

The solution mainly differs from the one outlined in #786 in the following ways:

  • multiple loggers can be specified (still optional, and probably not needed most of the time)
  • a tqdm class can be passed in
  • Python 2 support

@de-code de-code requested a review from casperdcl as a code owner April 1, 2021 19:35
@codecov
Copy link

codecov bot commented Apr 1, 2021 •
edited

Codecov Report

Merging #1155 (7458666) into devel (9fb0f23) will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            devel    #1155      +/-   ##
==========================================
+ Coverage   89.32%   89.80%   +0.48%     
==========================================
  Files          25       26       +1     
  Lines        1658     1707      +49     
  Branches      275      283       +8     
==========================================
+ Hits         1481     1533      +52     
+ Misses        131      128       -3     
  Partials       46       46

de-code
de-code commented Apr 1, 2021
View reviewed changes
tqdm/cli.py Outdated Show resolved Hide resolved
@de-code de-code mentioned this pull request Apr 1, 2021
Closed
@de-code de-code force-pushed the logging-redirect-to-tqdm branch 3 times, most recently from 72b8be1 to c24dab3 Compare April 2, 2021 09:54
casperdcl
casperdcl requested changes Apr 3, 2021
View reviewed changes
Copy link
Sponsor Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

this is great. Perhaps should be in tqdm.contrib.logging or even just tqdm.contrib.__init__ for now?

https://github.com/tqdm/tqdm/#redirecting-writing still uses tqdm.contrib for the stdout/err redirection.

@casperdcl casperdcl added p3-enhancement 🔥 Much new such feature question/docs ‽ Documentation clarification candidate submodule ⊂ Periphery/subclasses labels Apr 3, 2021
@casperdcl casperdcl added this to the Non-breaking milestone Apr 3, 2021
@casperdcl casperdcl added this to Next Release in Casper Apr 3, 2021
@de-code
Copy link
Contributor Author

de-code commented Apr 3, 2021

this is great. Perhaps should be in tqdm.contrib.logging or even just tqdm.contrib.__init__ for now?

https://github.com/tqdm/tqdm/#redirecting-writing still uses tqdm.contrib for the stdout/err redirection.

I moved it to tqdm.contrib.logging for now. It might be good to have a separate module just for the logging related functions.
I also moved the tests accordingly (there didn't seem to be any tests for contrib yet).
I also added a section to the README.

@de-code @casperdcl
added logging sub-module
e86b9b1 
added logging sub-module

python 2 compatibility

fixed python 2 fix

added test for custom tqdm class

python 2 absolute imports

(due to otherwise conflicting `logging` module)

isort

more tests relating to _get_first_found_console_formatter

isort

minor simplification

test handleError

test logging formatter being used

minor rename to _get_first_found_console_logging_formatter

test that certain exceptions are not swallowed

avoid using mock.assert_called (py 3.5)

moved to tqdm.contrib.logging

added "Redirecting console logging to tqdm" readme

removed no longer necessary absolute_import declaration

minor: updated package of example in docstring
@casperdcl casperdcl changed the base branch from master to devel April 5, 2021 22:46
@casperdcl
Copy link
Sponsor Member

fixes #786

@casperdcl casperdcl merged commit 3eeb09a into tqdm:devel Apr 5, 2021
Casper automation moved this from Next Release to Done Apr 5, 2021
@casperdcl casperdcl mentioned this pull request Apr 5, 2021
Merged
@de-code de-code deleted the logging-redirect-to-tqdm branch April 6, 2021 09:51
@casperdcl
Copy link
Sponsor Member

Hah @cthoyt it's a well overdue implementation of #786 - just never had enough free time to action it sooner; sorry. Full credit to @de-code though for one of the cleanest code PRs I've seen on this site.

user additions deletions
@de-code +429 -0
@casperdcl +102 -170

It may look like I modded a lot but most of that was documentation, not code.

@casperdcl casperdcl mentioned this pull request May 28, 2021
Open
@casperdcl casperdcl linked an issue May 28, 2021 that may be closed by this pull request
Pass progress bar to logger #313
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl requested changes

Assignees

@de-code de-code

@casperdcl casperdcl

Labels
p3-enhancement 🔥 Much new such feature question/docs ‽ Documentation clarification candidate submodule ⊂ Periphery/subclasses
Projects
Casper
  
Done
Milestone
Non-breaking
Development

Successfully merging this pull request may close these issues.

Pass progress bar to logger
3 participants
@de-code @casperdcl @cthoyt