Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Defaults added #26

wants to merge 7 commits into from

Conversation

bharel
Copy link

@bharel bharel commented Sep 12, 2024

closes #24

Pretty simple. Two unit tests to check for a defaults parameter alone, and with a rename.

Copy link
Owner

@nhairs nhairs left a 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.

src/pythonjsonlogger/core.py Outdated Show resolved Hide resolved
src/pythonjsonlogger/core.py Outdated Show resolved Hide resolved
src/pythonjsonlogger/core.py Outdated Show resolved Hide resolved
tests/test_formatters.py Show resolved Hide resolved
@nhairs
Copy link
Owner

nhairs commented Oct 21, 2024

Hi @bharel,

I plan to do a new point release as soon as #23 is ready (it is currently waiting on a new release of msgspec).

It would be good if this was ready to land in the same release.

nhairs and others added 4 commits October 22, 2024 15:07
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
@bharel bharel requested a review from nhairs October 22, 2024 14:44
@bharel
Copy link
Author

bharel commented Oct 23, 2024

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...?

************* Module pythonjsonlogger.msgspec
src/pythonjsonlogger/msgspec.py:20:0: E0401: Unable to import 'msgspec.json' (import-error)
************* Module pythonjsonlogger.orjson
src/pythonjsonlogger/orjson.py:20:0: E0401: Unable to import 'orjson' (import-error)

@nhairs
Copy link
Owner

nhairs commented Oct 24, 2024

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 🎉

Copy link
Owner

@nhairs nhairs left a 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.

pyproject.toml Outdated Show resolved Hide resolved
tests/test_formatters.py Show resolved Hide resolved
src/pythonjsonlogger/core.py Outdated Show resolved Hide resolved
@bharel
Copy link
Author

bharel commented Nov 2, 2024

Oh you managed to push it I see :-)

Can we close this then? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaults parameter is ignored
2 participants