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

Fix non-%-style formatting #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nonnull-ca
Copy link
Contributor

I dug into #22 a little, and it appears as though the problem is here:

    def __init__(self, fmt: str = '', datefmt: str = '', style: str = ''):
        super(DefaultFormatter, self).__init__(fmt=fmt, datefmt=datefmt)

        self._logger_tt_formatters = {}
        for case, fmt in self._standardize(fmt).items():
            self._logger_tt_formatters[case] = logging.Formatter(fmt=fmt, datefmt=datefmt, style=style)

Note that the initial super() call does not pass along style, but then it passes the same fmt into the underlying logging.Formatters with style. Then then results in the same format string being interpreted as %-style and {-style in different places.

Copy link
Owner

@Dragon2fly Dragon2fly left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your pull request.

Could you add a test case to make sure that the '{' formatting is working?
You could refer to the existing one to make your test case.
I'm using pytest and the test runs from the .\tests folder.

@nonnull-ca
Copy link
Contributor Author

Sure, shall do.

That being said, I'm hitting problems still. In particular, the 'automagicially'-instantiated root logger is instantiated in the import, which is necessarily before the call to basic_config. This then results in said root logger always using %-style formatting regardless of config... and because said root logger is, well, the root, this then results in all logging dying.

@nonnull-ca
Copy link
Contributor Author

In other words, this knocks down the first hurdle, not everything.

@nonnull-ca nonnull-ca mentioned this pull request May 7, 2023
@nonnull-ca
Copy link
Contributor Author

Ah. The QueueHandler is always using the default formatter. Let me see if I can fix that...

@nonnull-ca
Copy link
Contributor Author

As it turns out, style does not do what I think it did :/

It purely affects the style of the format string passed to the formatter, not the format of the log message itself.

There are still some fixes required for this to work - that I'll push in a few minutes along with an associated test - but looks like I'll still need to do my 'usual' wrapper to make {} work.

@nonnull-ca
Copy link
Contributor Author

======================================================================== test session starts ========================================================================
platform linux -- Python 3.11.2, pytest-7.1.2, pluggy-1.0.0
rootdir: [...]/logger_tt
plugins: xdist-3.2.1, timeout-2.1.0, hypothesis-6.29.3
collected 69 items                                                                                                                                                  

test_brace_style.py .                                                                                                                                         [  1%]
test_edit_log_path.py ..                                                                                                                                      [  4%]
test_handlers.py ....sss....                                                                                                                                  [ 20%]
test_inspector.py ..........s.s.........                                                                                                                      [ 52%]
test_multiprocessing.py ..........                                                                                                                            [ 66%]
test_recur_exception.py ..                                                                                                                                    [ 69%]
test_simple.py ....................                                                                                                                           [ 98%]
test_theading.py .                                                                                                                                            [100%]

============================================================= 64 passed, 5 skipped in 90.38s (0:01:30) ==============================================================

@nonnull-ca
Copy link
Contributor Author

Skipped tests were:

  1. The Telegram ones, as I do not have an account (nor do I wish one).
  2. test_multiline, which is skipped on Python 3.7+ and this is Python 3.11
  3. test_deadlock, which is marked as an unconditional skip (which is good, because exception_deadlock.py doesn't exist!)

Copy link
Owner

@Dragon2fly Dragon2fly left a comment

Choose a reason for hiding this comment

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

Thanks for your added code. Just a little change is needed before we can merge.

@@ -386,7 +387,7 @@ def _standardize(self, fmt):
formatters = {'normal': fmt.replace(self.default_formats['normal'][0], self.default_formats['normal'][1])}

# concurrent format
concurrent_fmt = formatters['normal'].replace('%(threadName)s', '').replace('%(processName)s', '')
concurrent_fmt = formatters['normal'].replace('%(threadName)s', '').replace('%(processName)s', '').replace('{threadName}', '').replace('{processName}', '')
Copy link
Owner

Choose a reason for hiding this comment

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

Hi, this line is too long. Can you split it into two lines?

concurrent_fmt = formatters['normal'].replace('%(threadName)s', '').replace('%(processName)s', '')
concurrent_fmt = concurrent_fmt .replace('{threadName}', '').replace('{processName}', '')

@@ -125,6 +125,7 @@ def _replace_with_queue_handler(self):
# add queue handler
queue = self.qclass()
q_handler = handlers.QueueHandler(queue)
q_handler.setFormatter(all_handlers[0].formatter)
Copy link
Owner

Choose a reason for hiding this comment

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

Here, we assume that all handlers use the same formatter.
It may not be always correct but let's just go with it for now.
I'll make a note in the README later.

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.

2 participants