-
Notifications
You must be signed in to change notification settings - Fork 8
feat: ✨ support DictConfigurator prefixes for rename_fields and static_fields #45
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
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.
First of all - this is very well put together PR 👌
Overall I'm onboard with the change.
I've left a few comments on the files. In addition I think we need to:
- Add something to the docs here - I'd be happy to mostly replace the
ini
example with youryaml
+dictConfig
example. - Add tests to show that the conversion is working as intended.
- Update the change log (if you add an
UNRELEASED
version header I'll handle links and versions etc later).
4a16466
to
ec424ae
Compare
@nhairs First of all, thank you for reviewing this PR. All suggested changes have been applied but test addition (as I really have no idea on how to do that). I simply tested they are working in my own project. It would be great if someone with more knowledge could add some. |
@nhairs Finally I managed to add a test to check dictConfig external reference support. Please check it as I'm not confident it is the best way to do it. |
74fce4a
to
646949b
Compare
646949b
to
22d0bb6
Compare
Yep I'll take a look 🙂 Also I'll note (in case we need more changes), I pretty much only do squash merges so don't fret too much about update history |
Hi @nhairs Did you have time to check out the new test for dictConfig external reference support? |
I've taken a quick look and overall everything looks okay - there's a few minor changes I'd like to push up but haven't had the chance |
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.
Alright I've made some minor wording changes in a couple of places.
Let me know if you're okay with my changes and I'll get this merged and look at doing a release soon.
Your changes are perfect. Thanks @nhairs |
Hi @nhairs Do you have any ETA for the new release? |
Hi @rubensa, That said I'm currently debating on whether this change is a breaking (major) or non-breaking (minor) and thus what type of version. One could argue that this is a bug that is being fixed and should be classed as a minor or even patch version. But, this has never been supported by the library and so using it might have unintended consequences for people who never expected it to handle the special prefixes, thus should be classified as a major version. |
Thanks for the update @nhairs As you can see (updated version from "3.3.0" to "3.3.1.dev0") I classed the change as a patch version (cause I thought it was a "mistake" in python-json-logger implementation details, preventing the already available feature in standard logging library to work as expected). I don't think that anyone might be using the prefixes expecting them to be "literal" (If anyone tried to use them I suppose that they were doing so, like me, expecting them to work like in standard logging library). That said, any version change you decide is ok to me. PS: I suppose that if you include this then it will be a major version change (cause it would change the way things are already defined to work). |
Hi @nhairs I'm sorry to bother you again but it's been over 2 weeks. I'm waiting for this feature to be available so I can include the library in my project. Do you have a new ETA for the release? |
Yeah understandable. I'm thinking it's probably a breaking release so there's some other stuff I want to look at included. Would a development release (on PyPI) be acceptable in the mean time? |
Ok. Let's do this with a development release for now. |
Hi @rubensa, sorry this took a few more days than I said. This is now uploaded: https://pypi.org/project/python-json-logger/4.0.0.dev0/ |
No worries. Thanks @nhairs |
Python logging config supports access to external and internal objects via DictConfiguator prefixes.
This works internally by warpping the dict with a ConvertingDict that requires an explicit call to it's __getitem__ dunder method for the conversion to take place (using the specific converter for the specified prefix).
This allows, for example, having a log_config.yaml file like:
where service, env and version values are taken from the external resource logging_config.py.
The content for logging_config.py could be something like (for getting some values from project metadata or environment variables):