Skip to content

Conversation

@galaunay
Copy link
Collaborator

Original solution from @akshaybadola here: #1868


"""
self.stdout.write(json.dumps(kwargs) + "\n")
self.stdout.write(json.dumps(kwargs, default=self.json_defaults) + "\n")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for json.dumps say that it should raise a TypeError if the object cannot be serializable, (In this case, all other objects that are not Path and are not primitive types will be encoded as null since json_defaults is implicitly returning None.

>>> import json
>>> json.dumps(None)
'null'
>>> 

Also relevant CPython source code that shows this will happen

I don't think this is the intended outcome?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for json.dumps say that it should raise a TypeError if the object cannot be serializable, (In this case, all other objects that are not Path and are not primitive types will be encoded as null since json_defaults is implicitly returning None.

>>> import json
>>> json.dumps(None)
'null'
>>> 

Also relevant CPython source code that shows this will happen

I don't think this is the intended outcome?

Hi @gopar. Sorry I've been extremely busy. Yes, definitely that shouldn't be there. I had completely overlooked that and forgot to return the default value. It should be:

def json_defaults(x):
    if isinstance(x, Path): 
         return str(x) 
    else: 
        return json.dumps(x)

See docstring of default in JSONEncoder.

Actually pydantic would be a good approach to go for it as it is primarily a parsing library. Though I think @gfederix's PR (See discussion in #1895) requires a few fixes.

At this point I'm not sure which things to refactor, for example in rpc.py, as there are parallel changes in #1895 which are not yet merged. Any suggestions?

@galaunay
Copy link
Collaborator Author

I merged #1916 instead of this one (the fix is the same, but the code looks cleaner).
We will revert it as soon as #1895 is ready !

@galaunay galaunay closed this Jun 30, 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.

4 participants