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

Consolidate JSON, JSONParseResults and JSONParser into JSON #44806

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 30, 2020

Following #44574 (comment).

Supersedes #44574
Part of #16863

Removes JSONParser and JSONParseResults.

Includes renaming:

  • JSON::parse_string() to parse()
  • JSON::decode_data() to stringify()

Note 1: stringify() returns the String instead of Error::Ok.
Note 2: Adds a to_json_string() method to Variant for use within the engine.

Edit: JSONParser has been renamed to JSON following PR Review meeting (comment)

@dalexeev
Copy link
Member

dalexeev commented Dec 30, 2020

Maybe rename the JSONParser to JSON?

In my opinion, the JSON singleton was more convenient, now you need to create a JSONParser instance every time.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 30, 2020

In my opinion, the singleton was more convenient, now you need to create a JSONParser instance every time.

I really think that we need support for defining static functions for scripting as provided by built-in classes, see my comments on this at godotengine/godot-proposals#1101 (comment) which also happens to mention JSON, this would solve the convenience issue...

@dalexeev
Copy link
Member

On the other hand, a new JSONParseResult instance was already created every time JSON.parse() was called. So the difference is not very big, but it is consistent with XMLParser.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44859 and #44911.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44989.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #45737.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #46200.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #46551.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #46713 and #46728.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #40804.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #48159, #48239, #47386 and #47974.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting today and approved consolidating those APIs in one place (again, I remember this PR followed a discussion in another meeting). The only change we'd suggest is that since the new JSONParser class is also used for writing (stringify), it should maybe renamed to just JSON. The other *Parser classes we have like VariantParser and XMLParser only do parsing, not writing.

@madmiraal madmiraal force-pushed the consolidate_json branch 2 times, most recently from d1ac567 to c35d4fc Compare June 19, 2021 06:33
Renames JSON.parse_string() to parse()
Renames JSON.decode_data() to stringify()
@madmiraal
Copy link
Contributor Author

Renamed JSONParser to JSON.

@madmiraal madmiraal changed the title Consolidate JSON, JSONParseResults and JSONParser into JSONParser Consolidate JSON, JSONParseResults and JSONParser into JSON Jun 19, 2021
@akien-mga akien-mga merged commit d88be9b into godotengine:master Jun 19, 2021
@akien-mga
Copy link
Member

Thanks!

@Faless
Copy link
Collaborator

Faless commented Jul 7, 2021

I don't understand why the engine exposed JSON::stringify is no longer static.
Having to write in engine every time:

JSON json;
json.stringify(...);

is quite awful.

@madmiraal
Copy link
Contributor Author

@Faless The PR includes the addition of a to_json_string() method to Variant for use within the engine.

Note 2: Adds a to_json_string() method to Variant for use within the engine.

godot/core/variant/variant.cpp

Lines 1842 to 1845 in 56d7126

String Variant::to_json_string() const {
JSON json;
return json.stringify(*this);
}

Note: If the variable is not a Variant but can be converted to one there is also this option: Variant(my_variable).to_json_string() for example:

Dictionary message = make_notification(p_method, p_params);
String msg = Variant(message).to_json_string();

@Faless
Copy link
Collaborator

Faless commented Jul 7, 2021

@madmiraal thanks for the clarification. I had totally missed it. My apologies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants