Conversation
597d75f to
c4d3f28
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4593 +/- ##
==========================================
- Coverage 97.85% 97.79% -0.06%
==========================================
Files 297 294 -3
Lines 15339 15200 -139
Branches 1721 1715 -6
==========================================
- Hits 15010 14865 -145
- Misses 188 190 +2
- Partials 141 145 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """A list of :class:`EventListener <.events.listener.EventListener>`.""" | ||
| logging_config: BaseLoggingConfig | None = field(default=None) | ||
| """An instance of :class:`BaseLoggingConfig <.logging.config.BaseLoggingConfig>` subclass.""" | ||
| logging_config: LoggingConfig = field(default_factory=LoggingConfig) |
There was a problem hiding this comment.
We can no longer pass None to this to disable the logging config?
There was a problem hiding this comment.
my app on this branch uses Litestar(logging_config=None) and I didnt have any errors
There was a problem hiding this comment.
Mhm. I think that's just an oversight in the typing. I'll check on that
There was a problem hiding this comment.
I think there are some performance implications here. IIRC, using None here can improve performance in some cases.
We should probably ensure the None remains an option here.
| def debug(self) -> bool: | ||
| return self._debug | ||
|
|
||
| @debug.setter |
There was a problem hiding this comment.
Is it intended so we can not change the debug status while the app is running or it was never working before?
There was a problem hiding this comment.
It has never properly worked before
There was a problem hiding this comment.
It's also very tricky to get right, because not all logging libraries work the same in regards of setting levels dynamically.
| return not self.pretty_print_tty | ||
| return True | ||
|
|
||
| def _get_structlog_config(self) -> dict[str, None]: |
There was a problem hiding this comment.
Litestar mostly use dict[str, Any] for returns type. It's the only line in the code where the returns is dict[str, None]. Is it intended?
There was a problem hiding this comment.
Nope. Any I'm not sure why mypy isn't complaining about this 😅
| exception_logging_handler: ExceptionLoggingHandler | None | ||
| """Handler function for logging exceptions.""" | ||
| disable_stack_trace: set[Union[int, type[Exception]]] # noqa: UP007 | ||
| disable_stack_trace: set[int | type[Exception]] = dataclasses.field(default_factory=set) |
There was a problem hiding this comment.
It would be nice to use this opportunity to set a few default exceptions to exclude from the stack trace.
I tend to always add the following default set to every application to cleanup logging output: {404, 401, 403, NotAuthorizedException, PermissionDeniedException}
| """This will be the configuration for the root logger. | ||
|
|
||
| Processing of the configuration will be as for any logger, except that the propagate setting will not be applicable. | ||
| always_propagate_on_pytest: bool = True |
| docs-serve: ## Serve the docs locally | ||
| @echo "=> Serving documentation" | ||
| uv run --isolated --python 3.12 sphinx-autobuild docs docs/_build/ -j auto --watch litestar --watch docs --watch tests --watch CONTRIBUTING.rst --open-browser --port=0 | ||
| uv run --isolated sphinx-autobuild docs docs/_build/ -j auto --watch litestar --watch docs --watch tests --watch CONTRIBUTING.rst --open-browser --port=0 |
There was a problem hiding this comment.
I think we should consider leaving these pins in.
When i install the dev env, I typically use the lowest supported version for the library (3.10 in this case), but the latest versions of Sphinx have a minimum version of 3.12. We have been running into similar problems with the Advanced Alchemy docs.
There was a problem hiding this comment.
I had a reason for unpinning this but I can't remember. I'll find out
Still need to clean up some things to reduce unnecessary breaking changes, but feature wise, everything is there.
capture_logs)