Allow run_app(access_log=None) #3504
Conversation
| handle_signals: bool=True, | ||
| reuse_address: Optional[bool]=None, | ||
| reuse_port: Optional[bool]=None) -> None: | ||
| """Run an app locally""" | ||
| loop = asyncio.get_event_loop() | ||
|
|
||
| # Configure if and only if in debugging mode and using the default logger | ||
| if loop.get_debug() and access_log.name == 'aiohttp.access': | ||
| if loop.get_debug() and access_log and access_log.name == 'aiohttp.access': |
There was a problem hiding this comment.
maybe
| if loop.get_debug() and access_log and access_log.name == 'aiohttp.access': | |
| if loop.get_debug() and getattr(access_log, 'name', None) == 'aiohttp.access': |
This comment has been minimized.
This comment has been minimized.
| @@ -269,7 +269,7 @@ | |||
| backlog: int=128, | |||
| access_log_class: Type[AbstractAccessLogger]=AccessLogger, | |||
| access_log_format: str=AccessLogger.LOG_FORMAT, | |||
| access_log: logging.Logger=access_logger, | |||
| access_log: Optional[logging.Logger]=access_logger, | |||
There was a problem hiding this comment.
why do u use Optional, and when access_log should be None?
There was a problem hiding this comment.
i just don't understand when u need to drop logging :)
There was a problem hiding this comment.
Firstly, this is just an adoption of the stubs to reflect existing docs, secondly I ask myself the vice versa question every time I try to get rid of some framework's logging. I have “dumb” HTTP logs in my load balancer and I want my applications to have only structured application logs.
There was a problem hiding this comment.
The load balancer is story about production, right? do u use debug mode for production?
Now, u can put your custom logger and this should solve your problem.
There was a problem hiding this comment.
This isn't about my problems, this is about making the code do what the docs say. :)
There was a problem hiding this comment.
so maybe the problem is not in the code and in the documentation :)
I don't find any test for this behavior and this feature never worked. I am not sure that this feature is required.
There was a problem hiding this comment.
It worked when debug was off which is the normal case. I ran into the edge case of debug=True thanks to mypy.
There was a problem hiding this comment.
You are right without mypy all is fine. I think will be cool have some test for this case.
|
Thanks! |
* Mark access_log as optional * Make access_log=None work in debug mode (cherry picked from commit c562ffe) Co-authored-by: Hynek Schlawack <hs@ox.cx>
psf-chronographer
bot
added
the
bot:chronographer:provided
What do these changes do?
The docs suggest that you can disable request logging by passing
access_log=Noneto run_app.However:
This PR fixes both.
Are there changes in behavior for the user?
Documented behavior works under more conditions now. :)
Checklist
CHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.