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

Log the arguments when running a task #24

Merged
merged 3 commits into from Oct 3, 2016
Merged

Log the arguments when running a task #24

merged 3 commits into from Oct 3, 2016

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Oct 1, 2016

Also log the command if run from the command line

@r-owen r-owen force-pushed the tickets/DM-7844 branch 3 times, most recently from 696a850 to 69b0fcc Compare October 2, 2016 21:48
Modify the argument parser to log the full details of the command.
If run from the command line (e.g. with no args from sys.argv)
then log the full command line.
If run by calling parseAndRun with args specified (e.g. a unit test)
then log <class>.parseAndRun(args).
Define a function instead of assigning a lambda
Reorder a few imports
argumentParser = cls._makeArgumentParser()
if config is None:
config = cls.ConfigClass()
parsedCmd = argumentParser.parse_args(config=config, args=args, log=log, override=cls.applyOverrides)
# print this message after parsing the command so the log is fully configured
parsedCmd.log.info("Running: {}".format(commandAsStr))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay but maybe better parsedCmd.log.info("Running: %s", commandAsStr) as log does the % formatting (and only the % formatting at this moment...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that delayed formatting is useful primarily when a log message may not be displayed. I would expect info and fatal to always be displayed. Still...it ought to be a harmless change and I guess it is good log style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does delayed %s formatting work for objects such as exceptions? Or must one specify a string representation of the object?

Copy link
Contributor

Choose a reason for hiding this comment

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

True, practically it only makes some differences in debugging messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same %s formatting, just done after checking the log level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick test shows that %s does work on arbitrary objects, as does %r. I just pushed a new commit that changes logging to use deferred formatting in all instances I found in pipe_base (which were all in cmdLineTask.py and argumentParser.py)

if not self.config.compare(oldConfig, shortcut=False, output=output):

def logConfigMismatch(msg):
self.log.fatal("Comparing configuration: " + msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, maybe delay the formatting to log?

@r-owen r-owen merged commit 54f122d into master Oct 3, 2016
@ktlim ktlim deleted the tickets/DM-7844 branch August 25, 2018 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants