Skip to content

bpo-34844: logging.Formatter enhancement - Ensure styles and fmt matches in logging.Formatter #9703

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.

Sign up for GitHub

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

Merged
vsajip merged 21 commits into from
Oct 15, 2018
Merged

bpo-34844: logging.Formatter enhancement - Ensure styles and fmt matches in logging.Formatter #9703

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
66bbddf
bpo-34844
BNMetrics Oct 4, 2018
0eaaf25
added news entry
BNMetrics Oct 4, 2018
f4f6a60
suggested changed before regex fix
BNMetrics Oct 5, 2018
d51e186
regex changes
BNMetrics Oct 6, 2018
cc2cfbd
commit
BNMetrics Oct 6, 2018
7c24e46
rewrote regex
BNMetrics Oct 6, 2018
abd868d
changes as requested
BNMetrics Oct 7, 2018
c1359bb
changes as requested
BNMetrics Oct 7, 2018
921ba33
added doc and docstring, added some of the original tests back for $ …
BNMetrics Oct 8, 2018
8484f29
changed validate method for StrFormatStyle
BNMetrics Oct 9, 2018
4c35216
changed on StrFormatStyle class
BNMetrics Oct 10, 2018
6eee298
commit
BNMetrics Oct 12, 2018
aab0c46
removed redundant debugging print
BNMetrics Oct 13, 2018
59879e9
added options to logging.config.dictConfig to allow passing in valida…
BNMetrics Oct 13, 2018
43c0b34
added in documentation for the validate key in logging.config.dictConfig
BNMetrics Oct 13, 2018
908c3ec
removing blank space
BNMetrics Oct 14, 2018
d73ab3a
rewrote logging.config.DictConfigurator.configure_formatter logic, si…
BNMetrics Oct 14, 2018
ff05742
document typo change on logging.rst
BNMetrics Oct 14, 2018
2664f85
reformatted logging.config.rst
BNMetrics Oct 14, 2018
74ca6a2
simplified the logic in logging.config.DictConfig.configure_formatter
BNMetrics Oct 15, 2018
de1243f
fixed document due to failure
BNMetrics Oct 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions Lib/logging/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,10 @@ def makeLogRecord(dict):
#---------------------------------------------------------------------------
# Formatter classes and functions
#---------------------------------------------------------------------------
_str_formatter = StrFormatter()
del StrFormatter


class PercentStyle(object):

default_format = '%(message)s'
Expand All @@ -438,7 +442,7 @@ def validate(self):
self._invalid_raise()

def _invalid_raise(self, additional_message=None):

@vsajip vsajip Oct 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would prefer the name _raise_invalid as the verb should come first, generally.

msg = 'Invalid format %s for %s style' % (self._fmt, self.default_format[0])
msg = "Invalid format '%s' for '%s' style" % (self._fmt, self.default_format[0])

@vsajip vsajip Oct 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can just use 'Invalid format %r for %r style' to get the single quotes in.

if additional_message:
msg = "%s, %s" % (msg, additional_message)

@vsajip vsajip Oct 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Try to use single quotes where possible.

raise ValueError(msg)
Expand All @@ -458,29 +462,38 @@ class StrFormatStyle(PercentStyle):
asctime_format = '{asctime}'
asctime_search = '{asctime'
# This is only for validating the format_spec
validation_pattern = re.compile(r"^((.)?[<>=]?[+ -]?[#0]?[\d]*,?[\.\d]*)?[bcdefgnosx%]?$", re.I)
validation_pattern = re.compile(r"^(.?[<>=^]?[+ -]?#?0?[\d]*,?(\.[\d]*)?)?[bcdefgnosx%]?$", re.I)

@vsajip vsajip Oct 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a bit of a think about this whole area - I think both brace and dollar validation need a bit more work. I put my thoughts into this Gist - see what you think and if you can adapt some of it. The test cases in there might be useful, too - and you could try adding to them using combinations I haven't tried.

I also think maybe we should add a validate= keyword parameter to Formatter.__init__, which defaults to True but which a user can set to False to disable validation, as a workaround for if we miss something.

It means that the validation_pattern idea I had was inadequate - for %-formatting a single pattern seems OK, for {-formatting we need the two-step approach you came up with, and I also realised that we hadn't catered for the no-fields case for $-formatting. But there's little point in having a uniform validate() that just relies on one pattern, as that won't fly. 😞

spec_fmt_pattern = re.compile(r"(?<=(?<!\{)\{)[^{}]*(?=\}(?!\}))")

def _format(self, record):
return self._fmt.format(**record.__dict__)

def validate(self):
"""Validate the input format, ensure it is the correct string formatting style"""
def _validate(self, fmt):
try:
fields = []
for _, field_name, spec, conversion in StrFormatter().parse(self._fmt):
for _, field_name, spec, conversion in _str_formatter.parse(fmt):

if field_name and "-" not in field_name:
if field_name:
if "-" in field_name:
raise ValueError("Invalid field name '%s', cannot have '-' in field name")
if conversion and conversion not in "rsa":
raise ValueError("invalid conversion %s for field %s" % (conversion, field_name))
if spec and not self.validation_pattern.search(spec):
raise ValueError("invalid spec %s for %s" % (spec[-1], field_name))
raise ValueError("invalid conversion '%s' for '%s' field" % (conversion, field_name))
if spec:
if self.spec_fmt_pattern.search(spec):
return self._validate(spec)
elif not self.validation_pattern.search(spec):
raise ValueError("invalid spec %s for '%s' field" % (spec[-1], field_name))

fields.append(field_name)
if not fields:
raise ValueError("No fields found for format %s" % self._fmt)
raise ValueError("no fields found")
except ValueError as e:
self._invalid_raise(e)

def validate(self):
"""Validate the input format, ensure it is the correct string formatting style"""
self._validate(self._fmt)


class StringTemplateStyle(PercentStyle):
default_format = '${message}'
Expand Down
60 changes: 53 additions & 7 deletions Lib/test/test_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3491,6 +3491,12 @@ def get_record(self, name=None):
result.update(self.variants[name])
return logging.makeLogRecord(result)

def assert_error_message(self, exception, message, *args, **kwargs):
try:
self.assertRaises(exception, *args, **kwargs)
except exception as e:
self.assertEqual(message, e.message)

def test_percent(self):
# Test %-formatting
r = self.get_record()
Expand Down Expand Up @@ -3564,8 +3570,12 @@ def test_format_validate(self):
self.assertEqual(f._fmt, "{customfield!s:#<30}")
f = logging.Formatter("{message!r}", style="{")
self.assertEqual(f._fmt, "{message!r}")
f = logging.Formatter("{process!s:<#30,12f}- {custom:=+#30,.1d} - {module:^30}", style="{")
self.assertEqual(f._fmt, "{process!s:<#30,12f}- {custom:=+#30,.1d} - {module:^30}")
f = logging.Formatter("{process!s:<#30,.12f}- {custom:=+#30,.1d} - {module:^30}", style="{")
self.assertEqual(f._fmt, "{process!s:<#30,.12f}- {custom:=+#30,.1d} - {module:^30}")
f = logging.Formatter("{process!s:{w},{p}}", style="{")
self.assertEqual(f._fmt, "{process!s:{w},{p}}")
# f = logging.Formatter("{process!s:{{w}},{{p})}", style="{")
# self.assertEqual(f._fmt, "{process!s:{{w}},{{p}}}")

# Dollar style
f = logging.Formatter("${asctime} - $message", style="$")
Expand All @@ -3584,14 +3594,50 @@ def test_format_validate(self):
self.assertRaises(ValueError, logging.Formatter, '%(foo)0*.8*f')

Comment thread
vsajip marked this conversation as resolved.
Resolved
# StrFormat Style
self.assertRaises(ValueError, logging.Formatter, "{name-thing}", style="{")
self.assertRaises(ValueError, logging.Formatter, '%(asctime)s', style='{')
self.assertRaises(ValueError, logging.Formatter, '{asctime!Z:15}', style='{')
self.assertRaises(ValueError, logging.Formatter, '{asctime!aa:15}', style='{')
self.assertRaises(ValueError, logging.Formatter, '{process:.2ff}', style='{')
# Testing failure for '-' in field name
self.assert_error_message(
ValueError,
"Invalid format '{name-thing}' for '{' style, Invalid field 'name-thing', cannot have '-' in field name",
logging.Formatter, "{name-thing}", style="{"
)
# Testing failure for style mismatch
self.assert_error_message(
ValueError,
"Invalid format '{name-thing}' for '{' style, no fields found",
logging.Formatter, '%(asctime)s', style='{'
)
# Testing failure for invalid conversion
self.assert_error_message(
ValueError,
"Invalid format '{asctime!Z:15}' for '{' style, invalid conversion 'Z' for 'asctime' field"
)
self.assertRaises(ValueError, logging.Formatter, '{asctime!s:#30,15f}', style='{')
self.assert_error_message(
ValueError,
"Invalid format '{asctime!aa:15}' for '{' style, expected ':' after conversion specifier",
logging.Formatter, '{asctime!aa:15}', style='{'
)
# Testing failure for invalid spec
self.assert_error_message(
ValueError,
"Invalid format '{asctime!Z:15}' for '{' style, invalid spect '.2ff' for 'process' field",
logging.Formatter, '{process:.2ff}', style='{'
)
self.assertRaises(ValueError, logging.Formatter, '{process:.2Z}', style='{')
self.assertRaises(ValueError, logging.Formatter, '{process!s:<##30,12g}', style='{')
self.assertRaises(ValueError, logging.Formatter, '{process!s:<#30#,12g}', style='{')
self.assertRaises(ValueError, logging.Formatter, '{process!s:{{w}},{{p}}}', style='{')
# Testing failure for mismatch braces
self.assert_error_message(
ValueError,
"Invalid format '{process' for '{' style, unmatched '{' in format spec",
logging.Formatter, '{process', style='{'
)
self.assert_error_message(
ValueError,
"Invalid format 'process}' for '{' style, unmatched '{' in format spec",
logging.Formatter, 'process}', style='{'
)

Comment thread
vsajip marked this conversation as resolved.
Resolved
# Dollar style
self.assertRaises(ValueError, logging.Formatter, '{asctime}', style='$')
Expand Down