I don't understand the thinking behind this. Why do we need a context manager for this? Please explain, See other comments further below.

I used a context manager to avoid duplication, however I like your solution better. I'm going to make the change :)

matches could mean it matches or it could mean all the matches. Suggest renaming to matched as it is used in a Boolean sense.

Before this PR, each of the format() methods is a one-liner which basically does return ... <result of formatting operation>. If you added a context manager to avoid duplicating the exception handling code [which it would be good to do], then it would be simpler to do as follows:

BEFORE:

in each XXXStyle class:

    def format(self, record):
        return ... <result of formatting operation>

AFTER:

In each XXXStyle class:

rename `def format(self, record)`  to `def _format(self, record)`

and in PercentStyle only (the base class for the others), define:

    def format(self, record):
        try:
            return self._format(self, record)
        except KeyError as e:
            raise ValueError('Formatting field not found in record: %s' % e)

With this approach, you avoid the duplication, without the need for a context manager, importing `contextlib` etc.

How did you arrive at this pattern? It doesn't seem to match the specification given here. It might be OK to match just the %(name)s part of the pattern, or any valid pattern, but not a halfway house. For example, your pattern matches %(name)Z and %(name)_, which are not valid for a format field specifier.

Hmm, this does seem to match the example you have given. let me try and redo this.

I think I have gotten quite a few things wrong in this pattern. I thought something like this would be valid%(asctime)- 15s, but looking at the specifications, I definitely got it wrong

How did you arrive at this pattern? It matches {name-thing}, which is not a great name for a key even though str.format() allows it if passing via a generic dictionary.

Seems reasonable, I will exclude some special characters in the keys

How did you arrive at this pattern? It matches ${funcName} but fails to match $funcName which is valid for a $-formatting operation.

I would add an extra test test_format_validation that ensures that all valid kinds of format strings (for all styles) pass validation (paying attention to the detailed specs for format specifiers for each style), as well as checking that invalid formats fail with ValueError.

This still has some problems:

• It is still not correct with respect to the spec - for example it doesn't handle the use of literal * to specify width and precision.
• It's needlessly verbose - [d|i|o|u|x|X|e|E|f|E|g|G|C|r|s|a|%]{1} could be replaced with just [diouxefgcrsa] if compiled with re.I- excluding the % character, as that wouldn't make sense as part of a %(name) format specifier, and the {1} is redundant.
• The bit between the parentheses - why not just use \w+?
• When specifying character alternatives, you don't need a | - [#0+ -] would be preferable to [#|0|\-|\s|+], leaving the - at the end to avoid ambiguity, as otherwise - inside [...] has special meaning.
• You didn't take up my point about using a compiled pattern. Did you not understand what I meant by that, or did you just forget it?

I suggest you just use re.compile(r'%\(\w+\)[#0+ -]?(\*|\d+)?(\.(\*|\d+))?[diouxefgcrsa]', re.I) as the validation_pattern.

You really don't need to use named values in the format string in this context. I gave a specific example in the earlier comment beginning "This would only be needed in PercentStyle:" - did you miss it? Also, you're not using a compiled pattern, as suggested.

I suggested earlier raise ValueError('Formatting field not found in record: %s' % e), as I thought that more informative. Do you not agree? "Invalid formatting field" suggests it might be improperly formatted, but the real problem is that it is not present in the record, and not how it's formatted (which your wording might be misinterpreted to mean).

See my earlier comments about using tidy, correct and minimalist regex patterns and using a compiled pattern. Those comments also apply here.

See my earlier comments about using tidy, correct and minimalist regex patterns and using a compiled pattern. Those comments also apply here. For example, your pattern here allows the invalid values $foo} and ${foo.

This is no longer needed - please remove.

ftm should be fmt.

This pattern still needs work 😞 For example, it passes {{levelname}} which contains no format specifiers: the doubling up of {{ means it is formatted as a single {. So '{{levelname}}'.format(levelname='foo') returns {levelname}, and we don't handle this. It's a little tricky: let me think about that part.

It also passes things like {leve*ln/ame} which doesn't seem right. I think you need to follow the spec more closely in the regex, even though it might feel like a pain to do. And these error cases above need to be added to the tests so that we can verify they are caught.

More problems:

• (.)? is best replaced by .?.
• We catch <, > and = in the align part, but not ^.
• According to the spec, that should be followed by #?0?, not [#0]? (i.e. one could have both chars present)
• [\d]* is equivalent to \d*, but noisier.
• We don't seem to allow _ as a grouping specifier.
• We perhaps need to handle {fieldname:{width}.{precision}} which is a valid specifier (note the }} at the end, which would normally be escaped - I'm thinking that maybe a regex won't do the job after all 😭).
• The precision specifier should be (\.\d+)?, not [\.\d]*.

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

Try to use single quotes where possible.

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. 😞

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

You probably didn't mean to leave this line in? Was it there just for debugging?

Yes! removing this now!

defaults -> a default

validate the correct format -> validate the format

IIRC importing inspect can be expensive, so consider only importing it if and where needed. If it is kept here, note that it needs to go in the above list in alphabetical order (I know other parts of logging don't follow that, but this module does). We may not even need it - see comment below.

This may not be needed, see above.

I might be overthinking this, but the logic in this area should perhaps be:

if not cname:
    c = logging.Formatter
else:
    c = _resolve(cname)  # this might actually resolve to logging.Formatter

if c is logging.Formatter:
    # we know c will accept the validate parameter
    use_validate = True
    validate = config.get('validate', True)
else:
    # we don't yet know if c will accept a validate parameter.
    use_validate = False
    # Here, we could go to some trouble to determine whether there's a validate
    # or kwargs parameter, as you've done. But we have to take care:  there are
    # a number of things to consider. We currently call c with fmt, dfmt, style
    # arguments. So we expect c to be a class which inherits from
    # logging.Formatter and either doesn't redefine __init__, or does redefine
    # it. In the first case, we'd be safe to set use_validate to True, but in
    # the second, we wouldn't, because we don't know if this a Formatter
    # subclass written after 3.8 was released - when the developer of it would
    # be expected to know about the validate parameter - or whether it was a
    # class designed before 3.8, when they wouldn't be expected to know about
    # it. If it's specified in the config, then they know about it and we can
    # assume it's a post-3.8 design, so we can pass the configured value;
    # otherwise, we have as yet no idea if the redefined __init__ will accept
    # validate. We could dig into its signature to see if we could figure it
    # out, but there's no guarantee it will be *our* validate parameter - even
    # if we can figure out that it's expected to be a bool, we have no idea
    # how a True value would be interpreted. So it's safest not to pass it.

    # In the case that c is a class which doesn't even inherit from Formatter,
    # we have no idea what its __init__ does, though we do know that it has to
    # take (fmt, dfmt, style) parameters, otherwise it wouldn't work now.
    # Thus:

    if issubclass(c, logging.Formatter):
        if '__init__' not in c.__dict__:  # not redefined
            use_validate = True
        elif 'validate' in config:  # user has specified it, so OK to use
            use_validate = True
        # else:
            # if we decided to implement signature checking, we would import
            # inspect here and introspect the signature. Note that you couldn't
            # rely on a parameter literally named 'kwargs' - someone could have
            # used **kw, for example. You'd have to rely on there being a
            # specific parameter p such that p.kind == inspect._VAR_KEYWORD.

if use_validate :
    result = c(fmt, dfmt, style)
else:
    result = c(fmt, dfmt, style, validate)

See comment above about formatter classes that don't inherit from logging.Formatter.

mismatch -> mismatched

There seems to be whitespace in this line and line 3311 causing make patchcheck to fail in CI. Please run make patchcheck locally that will fix this for you automatically. Patch output of make patchcheck for reference

diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py
index bfd801f600..9802955e6a 100644
--- a/Lib/test/test_logging.py
+++ b/Lib/test/test_logging.py
@@ -3305,10 +3305,10 @@ class ConfigDictTest(BaseTest):
         self.apply_config(config)
         handler = logging.getLogger("my_test_logger_custom_formatter").handlers[0]
         self.assertIsInstance(handler.formatter, ExceptionFormatter)
-
+
     def test_custom_formatter_class_with_validate3(self):
         self.assertRaises(ValueError, self.apply_config, self.custom_formatter_class_validate3)
-
+
     def test_custom_formatter_function_with_validate(self):
         self.assertRaises(ValueError, self.apply_config, self.custom_formatter_with_function)

Thanks

OK, much simpler than my overthinking! But that's not right, because if the user specified False in the config, then config.get('validate') would return False, so use_validate would not be set to True and the user's False would never be passed to the call.

IMO you could lose the use_validate altogether and simplify even further to:

if 'validate' in config:  # if user hasn't mentioned it, the default will be fine
    result = c(fmt, dfmt, style, config['validate'])
else:
    result = c(fmt, dfmt, style)

Notice my stubborn sticking to single quotes 😉

Insert "The validate parameter was added." before "Incorrect or mismatched style..."