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

Use Yaml instead of JSON #3426

Closed
wants to merge 1 commit into from
Closed

Conversation

hush-hush
Copy link
Member

What does this PR do?

JSON return unicode strings under Python2, even if this is a better
behavior it would be a breaking change compare to the previous version
of the Agent not using this lib to embed Python. Yaml returns bytes
under Python2 and Unicode under Python3. Since JSON is a subset of Yaml
we use 'yaml' here.

@hush-hush hush-hush added [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. changelog/no-changelog labels May 7, 2019
@hush-hush hush-hush added this to the 6.12.0 milestone May 7, 2019
@hush-hush hush-hush force-pushed the maxime/use-yaml-instead-of-json branch 2 times, most recently from da73123 to 67a5c8c Compare May 9, 2019 21:07
JSON return unicode strings under Python2, even if this is a better
behavior it would be a breaking change compare to the previous version
of the Agent not using this lib to embed Python. Yaml returns bytes
under Python2 and Unicode under Python3. Since JSON is a subset of Yaml
we use 'yaml' here.
@hush-hush hush-hush force-pushed the maxime/use-yaml-instead-of-json branch from 99f2dac to bacf7a0 Compare May 10, 2019 17:54
// of the Agent not using this lib to embed Python. Yaml returns bytes
// under Python2 and Unicode under Python3. Since JSON is a subset of Yaml
// we use 'yaml' here.
char module_name[] = "yaml";
Copy link
Member

@arbll arbll May 14, 2019

Choose a reason for hiding this comment

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

JSON is indeed a subset of YAML 1.2. However I am not 100% sure if pyyaml implements 1.2 guidelines:
yaml/pyyaml#116

YAML 1.2 changes were mainly about dealing with some corner case that made YAML "kind of" a superset of JSON. diff are highlighted here: https://github.com/yaml/yaml/wiki/YAML-1.2-Changelog. I think there is a chance we might hit some of them. WDYT ?

Copy link
Member Author

@hush-hush hush-hush May 14, 2019

Choose a reason for hiding this comment

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

We completely control the data between the agent and six. It's serialized by the agent and loaded by six so we know that escaped / would not happened (which is the only one we care about).

But you raise a good point, since we control the whole chain why not use YAML everywhere !

I open another PR (#3468) with the whole change.

@masci @arbll Tell me what you think between the two.

@arbll
Copy link
Member

arbll commented May 20, 2019

Will be merged as part of #3468

@arbll arbll closed this May 20, 2019
@hush-hush hush-hush deleted the maxime/use-yaml-instead-of-json branch July 30, 2019 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants