Skip to content

function to jsonize prophy structs - #23

Open
mareklabonarski wants to merge 7 commits into
aurzenligl:masterfrom
mareklabonarski:master
Open

function to jsonize prophy structs#23
mareklabonarski wants to merge 7 commits into
aurzenligl:masterfrom
mareklabonarski:master

Conversation

@mareklabonarski

Copy link
Copy Markdown

No description provided.

Comment thread prophy/tests/test_utils.py Outdated
Comment thread prophy/tests/test_utils.py Outdated
Comment thread prophy/tests/test_utils.py Outdated
('dynamic', [
[('re', 45), ('im', 55)]
]),
]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add tests for serializing pure python's list, dict and tuple also test empty arguments for each case. Check strings with unicode characters and bytes strings with non ASCII characters (e.g. with each of '\x00\x01\x0a\xaf\xff').

Comment thread prophy/tests/test_utils.py
Comment thread prophy/utils.py Outdated
Comment thread prophy/utils.py Outdated
Comment thread prophy/tests/test_utils.py Outdated
Comment thread prophy/six.py Outdated

@aurzenligl aurzenligl left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just a bunch of comments; it would be nice if purpose of PR was laid out in description - now it's just an addition of module which is not used in prophy lib or prophyc app.

Comment thread prophy/utils.py
return str(e)


def jsonize(struct_, ordered=True):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Did you give yaml a thought? It seems to be "best suited for configuration" [1], and this is rather configuration (schema/typesystem of messages) than data (actual message objects with values). It's a little lighter than json for human eyes, allows comments, but might be less popular in terms of available parsers, etc.

>>> print(json.dumps(data, indent=True))
{
 "foo": {
  "x": [
   1,
   2,
   3
  ],
  "y": "uhishfusd",
  "z": 1203
 }
}
>>> print(yaml.dump(data))
foo:
  x: [1, 2, 3]
  y: uhishfusd
  z: 1203

Can you point out disadvantages of yaml that I fail to see while writing this? :)

[1] https://www.json2yaml.com/yaml-vs-json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good point. yaml is much more readable than json. I think the reason to implement such exporter is that JSON can be used in Java Script out of the box - while YML needs some external libraries. I think exposing methods for both would be nice.

@mareklabonarski mareklabonarski Mar 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yaml is nice, and good to have here. But since it seems so easy to change json-> yaml (yaml.dump(json_struct)), do we need anything in our library? That can be done externally on the flow...

@kamichal kamichal Mar 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No it's not needed. Even @aurzenligl did it in this way in his example (yaml.dump(..)). However there is one reason to make custom yaml frontend: hidden fields. Comments in yaml could help avoiding a confusion. E.g. a unpopulated dynamic array is not rendered in such a json, inactive optional field as well. A comment mentioning that fact could be very desirable.

@aurzenligl aurzenligl Mar 15, 2019

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If function aims at preparing data structures for further serialization in json/yaml, name: sanitize_* could be used. It doesn't mention serialization itself nor serialization format, but it suggests conforming to some stricter standard. Jsonize suggests making and returning json.

Comment thread prophy/utils.py Outdated
Comment thread prophy/utils.py Outdated
Comment thread prophy/utils.py Outdated
Comment thread prophy/prophy_six.py Outdated
@aurzenligl

Copy link
Copy Markdown
Owner

@mareklabonarski, where are we with this PR, would you like to have it in prophy, or should we close it?

@mareklabonarski

Copy link
Copy Markdown
Author

Please let me keep it open and come back to it once all higher priority things are done.
This MR is unrelated to the recent ones and part of different task, which waits for its time to be finished..

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