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

ISSUE-1585 | Surface fontTools logs for easier investigation #2169

Closed
wants to merge 1 commit into from

Conversation

kdipippo
Copy link

@kdipippo kdipippo commented May 28, 2024

@liZe
Copy link
Member

liZe commented Jun 9, 2024

After thinking about the best way to solve this issue, here’s what seems to be the best solution for me.

The code needed to capture logs is already in WeasyPrint: it’s used for tests, to check that we get no warning or the intended warning messages, depending on the test.

class CallbackHandler(logging.Handler):
"""A logging handler that calls a function for every message."""
def __init__(self, callback):
logging.Handler.__init__(self)
self.emit = callback
@contextlib.contextmanager
def capture_logs():
"""Return a context manager that captures all logged messages."""
logger = LOGGER
messages = []
def emit(record):
if record.name == 'weasyprint.progress':
return
messages.append(f'{record.levelname.upper()}: {record.getMessage()}')
previous_handlers = logger.handlers
previous_level = logger.level
logger.handlers = []
logger.addHandler(CallbackHandler(emit))
logger.setLevel(logging.DEBUG)
try:
yield messages
finally:
logger.handlers = previous_handlers
logger.level = previous_level

What we could do is:

  • Move CallbackHandler and capture_logs to logger.py.
  • Add a parameter to capture_logs to define the logger we want to capture. Its default value would be 'weasyprint' to keep the current default behaviour.
  • Use with capture_logs('fontTools') as logs: around the call to subsetter.subset() and display the logs thanks to LOGGER.warning(), just as you did in your PR.

What do you think of this solution?

As the code has changed a bit in main, you should probably create a new branch from main, create a new commit and push force to your branch.

@liZe liZe closed this in 61852c4 Jul 3, 2024
@liZe
Copy link
Member

liZe commented Jul 3, 2024

I’ve fixed the problem in 61852c4. Thanks a lot for your investigation and for your proposal. 💜

@liZe liZe added this to the 63.0 milestone Jul 3, 2024
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