-
-
Notifications
You must be signed in to change notification settings - Fork 723
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
Revert logging
file name modification
#1543
Conversation
* Modified logging module name to avoid clash with stdlib * Renamed _logging to indicate privacy
yep, I'd prefer we add a note to say that change will crash the custom logger, it crashed mine too, just need to change the name inside the custom yaml config from |
But then... We are making the user access a private module (even if not directly), and motivating them to do it? 🤔 |
fair enough, e could also put the formatters in a public uvicorn.formatters and let the _logging module private ? |
Yes, let's keep a publicly-accessible place for the logging helpers we expose to users. I stumbled upon this myself trying to copy this over... #491 (comment) after upgrading to 0.18.1. For minimum disruption, I would say we could:
It's also possible #1426 wasn't a good idea in the first place, and this PR is legit in the current state. In what situations was the name In all cases, I think we should add a test that imports the items meant for public logging API -- the ones that would end up in strings in logging configuration files. |
Naming the module |
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.
Based on this discussion I'm also in favor of straight-up reverting #1426. 👍
Adding a small "try to import logging stuff" test case would make this PR even better, but IMO it's acceptable in its current state.
I'm no expert, but changing |
IIUC doing You'd need to do That's why I was confused and asked in what cases the name clashing which motivated #1426 would occur. |
iirc some people were running |
I didn't get what test you want. Can you rephrase/exemplify? |
@Kludex Ith ink it's about to add a quick test that uses "uvicorn.logging". |
Something like this: def test_logging_api() -> None:
# These items may be referenced by users in logging configurations.
from uvicorn.logging import DefaultFormatter # noqa
from uvicorn.logging import AccessFormatter # noqa |
The I think it may be worth mentioning submodule renames in the release notes, too. |
Uvicorn changed back to `logging`: encode/uvicorn#1543
I'll make sure to mention those if it happens again. Thanks @polyrand :) |
Thank you all for the awesome work ❤️ |
This commit will add a pytest parameter that verifies log message output after configuring logging with the Uvicorn log format. This parameter is important for testing not only for the log message format, but also the use of the `uvicorn.logging.DefaultFormatter` class and its module. The module has been renamed from `uvicorn.logging` to `uvicorn._logging`, then reverted back to `uvicorn.logging`. The test will help ensure that the Uvicorn module path (`uvicorn.logging.DefaultFormatter`) is correct. encode/uvicorn#1543
Reported on b48d32d#commitcomment-76948401.
The problem is that users are relying on configuration files like that one.
_logging.py
shouldn't be private module if they are using it. Also, I do think it's meant for users to do that. 🤔Thoughts here @euri10 ?