Skip to content

journal: add extra parameter to JournalHandler constructor #52

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

Closed
wants to merge 1 commit into from

Conversation

otamachan
Copy link
Contributor

Hello,

logging.config.fileConfig supports only args which is a list of positional arguments.
https://docs.python.org/3/library/logging.config.html#configuration-file-format
But JournalHandler currently accepts only keyword arguments.

This change enables to add extra fields to JournalHandler in a configuration file.

class=systemd.journal.JournalHandler
args=(INFO, {'SYSLOG_IDENTIFIER': 'my-cool-app'})

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.

super(JournalHandler, self).__init__(level)

if isinstance(extra, dict):
kwargs = extra
Copy link
Member

Choose a reason for hiding this comment

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

The patch looks good, with the exception of this part. extra completely overrides kwargs. I think it would be better to simply look at both:

for d in (extra, kwargs):
   for name in d:
       if not _valid_field_name(name):
             ...
       ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change enables to add extra fields to JournalHandler
in a configuration file loaded by `logging.config.fileConfig`.

------
class=systemd.journal.JournalHandler
args=(INFO, {'SYSLOG_IDENTIFIER': 'my-cool-app'})
@keszybz
Copy link
Member

keszybz commented Nov 12, 2020

Merged as 5dd8afd. When merging this, I realized that the patch would break backwards compatibility by inserting a new positional argument. I tweaked your patch to add a new constructor classmethod instead. I hope that's OK. Thank you for the patch and sorry for the delay.

@keszybz keszybz closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants