Address todos / review comments

Author: nhairs

src/pythonjsonlogger/core.py

@@ -184,7 +184,7 @@ def __init__(
         """
         ## logging.Formatter compatibility
         ## ---------------------------------------------------------------------
-        # Note: validate added in 3.8, defaults added in 3.10
+        # Note: validate added in python 3.8, defaults added in 3.10
         if style in logging._STYLES:
             _style = logging._STYLES[style][0](fmt)  # type: ignore[operator]
             if validate:
@@ -196,6 +196,8 @@ def __init__(
             self._style = style
             self._fmt = fmt
 
+            # TODO: Validate comma format
+
         else:
             raise ValueError(f"Style must be one of: {','.join(logging._STYLES.keys())}")
 
@@ -278,15 +280,9 @@ def parse(self) -> List[str]:
             list of fields to be extracted and serialized
         """
         if self._fmt is None:
-            # TODO: does it matter that we do this before checking if the style is valid?
-            # (we already (mostly) check for valid style names in __init__
             return []
 
         if isinstance(self._style, str) and self._style == ",":
-            # TODO: should we check that there are no empty fields?
-            # If yes we should do this in __init__ where we validate other styles?
-            # Do we drop empty fields?
-            # etc
             return [field.strip() for field in self._fmt.split(",") if field.strip()]
 
         if isinstance(self._style, logging.StringTemplateStyle):

tests/test_formatters.py

@@ -158,7 +158,7 @@ def test_default_format(env: LoggingEnvironment, class_: type[BaseJsonFormatter]
 
 @pytest.mark.parametrize("class_", ALL_FORMATTERS)
 def test_percentage_format(env: LoggingEnvironment, class_: type[BaseJsonFormatter]):
-    # Note: All kind of different %s styles to check the regex
+    # Note: We use different %s styles in the format to check the regex correctly collects them
     env.set_formatter(class_("[%(levelname)8s] %(message)s %(filename)s:%(lineno)d %(asctime)"))
 
     msg = "testing logging format"