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

Issue #407: Added capabilty to monitor progress with a logger #408

Merged
merged 4 commits into from Aug 6, 2019

Conversation

@dusktreader
Copy link
Contributor

commented Jan 20, 2017

Adds a log_progress function, resolves #407.

@alimanfoo
Copy link
Collaborator

left a comment

Looks great, just a couple of minor suggestions for clarity.

@@ -35,16 +37,49 @@ def progress(table, batchsize=1000, prefix="", out=sys.stderr):
return ProgressView(table, batchsize, prefix, out)


def log_progress(table, batchsize=1000, prefix="", out=None, level=logging.INFO):

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 24, 2017

Collaborator

Suggest to rename 'out' to 'logger' just to help make it clear that a logger is expected here.

This comment has been minimized.

Copy link
@dusktreader

dusktreader Jan 24, 2017

Author Contributor

OK. Left it out so that it agreed with the progress() function, but this makes sense, too.

@@ -35,16 +37,49 @@ def progress(table, batchsize=1000, prefix="", out=sys.stderr):
return ProgressView(table, batchsize, prefix, out)


def log_progress(table, batchsize=1000, prefix="", out=None, level=logging.INFO):
"""
Report progress on rows passing through. E.g.::

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 24, 2017

Collaborator

Suggest 'Report progress on rows passing through to a logger.'

This comment has been minimized.

Copy link
@dusktreader

dusktreader Jan 24, 2017

Author Contributor

Yep, no problem

"""
Report progress on rows passing through. E.g.::
Report progress on rows passing through to a log. E.g.::

This comment has been minimized.

Copy link
@alimanfoo

alimanfoo Jan 24, 2017

Collaborator

Suggest 'Report progress on rows passing through to a file or file-like object (defaults to sys.stderr).'

@bmaggard

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

FWIW, I solved this problem by wrapping the logger and passing it to out. Using a dependency injection model avoids having to, e.g. s/progress/log_progress/g when switching modes. I'm not sure whether the code is entirely correct, but it seems to work:

class LoggerWriter:
    """Redirect stdout and stderr to logger.

    http://stackoverflow.com/a/31688396

    e.g.:
        log = logging.getLogger('foobar')
        sys.stdout = LoggerWriter(log.debug)
        sys.stderr = LoggerWriter(log.warning)

    But the real reason I needed this was so that petl could write
    progress to the log:

    p = p.progress(1000000, out=LoggerWriter(logger.info))
    """
    def __init__(self, level):
        # self.level is really like using log.debug(message)
        # at least in my case
        self.level = level

    def write(self, message):
        # if statement reduces the amount of newlines that are
        # printed to the logger
        if message != '\n':
            self.level(message)

    def flush(self):
        # create a flush method so things can be flushed when
        # the system wants to. Not sure if simply 'printing'
        # sys.stderr is the correct way to do it, but it seemed
        # to work properly for me.
      # self.level(sys.stderr)
      # I'm trying it without, because I don't like all the clutter
        pass
@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2017

@dusktreader

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

@alimanfoo I've updated the PR with your suggested changes

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2017

@dusktreader

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@alimanfoo Have you had a chance to look at this yet?

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2017

@dusktreader

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

@alimanfoo no problem. Just checking in

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2017

@dusktreader

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

@alimanfoo Looks like I've been plenty busy, too. Again, just checking in. Thanks!

@dusktreader

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2018

@alimanfoo So, it seems like petl is dead in the water. I think we'll be transitioning to another library, unfortunately.

@alimanfoo

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2018

@coveralls

This comment has been minimized.

Copy link

commented Aug 6, 2019

Coverage Status

Coverage increased (+1.2%) to 91.087% when pulling 84d6d0a on dusktreader:issue_407 into 3a61ad9 on petl-developers:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Aug 6, 2019

Coverage Status

Coverage increased (+1.2%) to 91.087% when pulling 84d6d0a on dusktreader:issue_407 into 3a61ad9 on petl-developers:master.

@alimanfoo alimanfoo merged commit 014746c into petl-developers:master Aug 6, 2019

@alimanfoo alimanfoo added this to the v1.3 milestone Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.