-
Notifications
You must be signed in to change notification settings - Fork 6
Add logging compatibility layer #188
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
base: main
Are you sure you want to change the base?
Conversation
The necessary changes to antsibull-build and antsibull-docs can be found in ansible-community/antsibull-build#666 and ansible-community/antsibull-docs#414. |
I've been really busy, but I'll get to this as soon as I can |
Sorry for the delay... I will take a closer look at the diff but the overall approach seems reasonable. A couple of comments:
|
pass | ||
|
||
@abc.abstractmethod | ||
def notice(self, format_spec: str, *args, **kwargs) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice()
seems unique to twiggy; I guess we can just have this method call info()
with the same arguments when/if we migrate.
That sounds interesting. Unfortunately it only supports printf-style formatting.
It doesn't seem to be that hacky, by using own log messages it should work fine. This also allows to do structured logging, and write log handlers that handle the structured data.
That's something I really want to avoid. Originally antsibull was using printf-style formatting for logging IIRC, and I was very happy that it was possible to get rid of it. I really don't want to go back to it. |
35b76af
to
ec1137e
Compare
@gotmax23 how can we move forward with this? |
I don't have a huge issue with printf-style formatting, and I remember reading that it's more performant than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look at all the docstring changes in detail, but the code looks okay to me.
I've looked a bit into replacing twiggy with the standard logging library in antsibull-docs. While this is rather easy to implement (using infos from https://stackoverflow.com/a/75715402 to allow using
{}
formatting instead of%
formatting in log messages), it would be quite a breaking change since suddenly the log output goes somewhere else and looks differently - and we'd need to parse the twiggy config from the app context to avoid some of the breaking changes.That's quite some work, and work I don't really want to do, so I tried another approach: provide some logging warpper in antsibull-core that seamlessly replaces twiggy's logger - and for now simply uses it -, so that (in a later edition) the user can change the configuration to use the standard logging interface instead of twiggy, without any changes to the antsibull-core users (antsibull-build and antsibull-docs). Then for antsibull-core 4.0.0, we can remove twiggy support and force-migrate everyone to the "new" standard logging.
Ref: #39