Ensure that log messages use sprintf-style formatting, not format.

[bburan]

Sprintf-style formatting is faster than str.format. Also, str.format always gets called even if the logging message never gets used, whereas sprintf-style formatting only gets invoked if the logging message is needed (e.g. it's getting sent to a file or the console).

[svdavid]

can you point to an example of where you use the improved form?

[bburan]

log.debug('This is my error message called %s with %.02f error', name, error)

[svdavid]

You can do that??? --without an explicit sprintf call? I agree, this makes a lot of sense.

[jacobpennington]

Inside log statements you can, don't think it works anywhere else (unless some other package supports it obviously).

[neheller]

I think that this grep command will find all of them

grep -ER $'(log|logging)\.(warn|info|exception|debug|log|critical|warning)\((\'|\").*(\'|\")\.format'

[bburan]

This is great. Thanks! I wonder if there's a way to add a pre-commit hook that rejects commits if someone used "format" in a logging message?

…

On Tue, Apr 10, 2018 at 1:47 PM, Nick Heller ***@***.***> wrote: I think that this grep command will find all of them grep -ER $'(log|logging)\.(warn|info|exception|debug|log|critical|warning)\((\'|\").*(\'|\")\.format' — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <LBHB/NEMS#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT5Qkk-1ExSw2a8oB3spU9-a-AaN6plks5tnRpmgaJpZM4S0vdo> .

[neheller]

I'm not sure there's a simple way to achieve this. A pre-commit client side hook would be fairly easy to set up, but there's no way to enforce that all contributors enable it. GitHub has a web hook api which can detect pull requests, but we would need to set up a service somewhere that scrapes the added code and tests it.

[svdavid]

Could it be built into a test function?

[neheller]

It could but unless I'm misunderstanding how you're using tests, this wouldn't prevent commits with the .format style either. If we're okay with making the check 'opt-in' then I think a pre-commit hook is the best option.

Here's a hook I threw together for this. It tests any added or modified line for the regex in question. If you want to implement it, just set it down as /.git/hooks/pre-commit (no file extension) in this repository and make sure it has executable permissions.

#!/bin/bash

if git rev-parse --verify HEAD >/dev/null 2>&1
then
	against=HEAD
else
	# Initial commit: diff against an empty tree object
	against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi


numbad=`git diff --cached $against | grep -E $'^\+.*(log|logging)\.(warn|info|exception|debug|log|critical|warning)\((\'|\").*(\'|\")\.format' | wc -l`

echo $numbad style errors found

# If you want to allow bad style, set this variable to true
allowbadstyle=$(git config --bool hooks.allowbadstyle)

if [ "$allowbadstyle" != "true" ] && [ $numbad -gt 0 ]
then
  git diff --cached $against | grep --color=auto -E $'^\+.*(log|logging)\.(warn|info|exception|debug|log|critical|warning)\((\'|\").*(\'|\")\.format'
  cat <<\EOF

Error: Using prohibited formatted string for logging

Example of incorrect usage:

logging.warn("This is an example {}".format(expl))

Example of correct usage:

logging.warn("This is an example %s", expl)

If you know what you are doing you can disable this check using:

  git config hooks.allowbadstyle true

EOF
  exit 1
fi

exit 0

[neheller]

Found a bug in the above script, you should use the current (edited) version.