Skip to content

Implement extra properties from json-logging-py #78

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

Merged
merged 4 commits into from
Jul 9, 2021

Conversation

uda
Copy link
Contributor

@uda uda commented Jun 29, 2021

Addresses feature request #77

Taken from: https://github.com/sebest/json-logging-py/blob/master/jsonlogging.py#L40
Original work by Sebastien Estienne [email protected]
Distributed under the MIT license

@bobbui
Copy link
Owner

bobbui commented Jul 2, 2021

thanks for your contribution, there is one test is failing, can u take a look

@alexhafner
Copy link

basestring is a Python 2 type, in Python 3 there is only str. The code from json-logging will need to be updated to Python 3.

@bobbui
Copy link
Owner

bobbui commented Jul 3, 2021

ideally the this module is built to support both py2 and py3 . I see this

if sys.version_info < (3, 0):
    EASY_TYPES = (basestring, bool, dict, float, int, list, type(None))
else:
    RECORD_ATTR_SKIP_LIST.append('stack_info')
    EASY_TYPES = (str, bool, dict, float, int, list, type(None))

Isnt it supposed to be working for both py2 and py3

@alexhafner
Copy link

Ah, didn't see that you still support Python 2, as the test matrix is only targeting 3.7 to 3.9

In this case flake8 could be instructed to disable the check for this specific line, ie

if sys.version_info < (3, 0):
    EASY_TYPES = (basestring, bool, dict, float, int, list, type(None)) # noqa: F821
else:
    RECORD_ATTR_SKIP_LIST.append('stack_info')
    EASY_TYPES = (str, bool, dict, float, int, list, type(None))
    ```

@bobbui
Copy link
Owner

bobbui commented Jul 4, 2021

@uda Can u add suggested flake8 fix to get it passed

@uda
Copy link
Contributor Author

uda commented Jul 4, 2021

Yeah, I can remove all Python 2 BC code or put comments to fix the flake8 test

@bobbui
Copy link
Owner

bobbui commented Jul 4, 2021

Yeah, I can remove all Python 2 BC code or put comments to fix the flake8 test

flake8 would be great

@uda
Copy link
Contributor Author

uda commented Jul 9, 2021

I added a fix for the basestring definition in python 3, I've seen it in another library with backwards compatibility and flake8 tests where they didn't want to simply ignore F821.
But when I run the second flake8 test it failed, but we'll see how it performs on github's pipeline

@bobbui
Copy link
Owner

bobbui commented Jul 9, 2021

looking good

@bobbui bobbui merged commit 8d6c1dd into bobbui:master Jul 9, 2021
@bobbui
Copy link
Owner

bobbui commented Jul 9, 2021

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.

3 participants