-
Notifications
You must be signed in to change notification settings - Fork 5
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
Defaults added #26
Defaults added #26
Conversation
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.
Overall looks good.
I'm wondering if we should add something to the quick start docs.
Improves handling of optional packages by: - No importing them just to check if available - Raises a more specific type of error (and message) ### Test Plan - Run unit tests
# Conflicts: # docs/changelog.md
Haven't run black on the last change, I apologize. Should be good now. I'm receiving import errors on mypy, but those are unrelated (running on py3.13) where msgspec doesn't exist yet...?
|
Yep that's fine - on CI we run it using 3.12 which I can see is passing 🎉 |
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 think we still need to add something to the "Quick start" docs, we also need to add to the doctstring change log.
~If I'm able to push to the branch I'll do these change myself to save on time. ~ I cannot push.
Could you add the following to docs/quickstart.md
before the Static Fields
section:
#### Default Fields
Default fields that are added to every log record prior to any other field can be set using the `default` argument.
```python
formatter = JsonFormatter(
defaults={"remote_ip": None}
)
# ...
logger.info("this overwrites the remote_ip field", extras={"remove_ip": "1.1.1.1"})
```
And the following to the BaseFormatter
docstring:
*Changed in 3.2*: `defaults` argument is no longer ignored.
Oh you managed to push it I see :-) Can we close this then? :D |
closes #24
Pretty simple. Two unit tests to check for a defaults parameter alone, and with a rename.