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

Rename JSON::print() to to_json() #44574

Closed
wants to merge 1 commit into from

Conversation

madmiraal
Copy link
Contributor

As originally identified here, currently, JSON::print() doesn't print anything, it converts the specified Variant to a JSON String. Since, other references to this method in Godot -- including the @GDscript version of this method (in 3.2) -- that call this method are all to_json(), it makes sense to rename this method to_json().

Part of #16863.

@nathanfranke
Copy link
Contributor

to_json implies there is a method from_json. I think either

  • parse->from_json
    or
  • print->stringify

would suffice better

@dalexeev
Copy link
Member

dalexeev commented Dec 22, 2020

@aaronfranke:

Since this class only has two methods for going between JSON and Variant types, I would suggest against methods containing the word json since it's pointless.

@bojidar-bg:

stringify has the benefit of being better-known among developers due to being used in JavaScript, however

I don't think the name should be consistent with the GDScript function, especially since it is planned to remove it. At the same time, it makes sense to be consistent with JavaScript (parse and stringify) because JSON is JavaScript Object Notation.

@dalexeev
Copy link
Member

Also, JSON.encode and JSON.decode are good options.

@nathanfranke
Copy link
Contributor

nathanfranke commented Dec 22, 2020

Also, JSON.encode and JSON.decode are good options.

With those names, though, it is not immediately obvious which one does which. stringify is super obvious that it returns a JSON string since it's in the method name. I would also be happy with JSON::to_string, but that could conflict with Object::to_string.

@aaronfranke
Copy link
Member

parse and stringify make sense to me, and if it's consistent with JavaScript that's great too. All options are better than print.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting today, basically we now have two different approaches to parsing JSON:

  • JSON singleton which returns a JSONParseResult
  • JSONParser similar to XMLParser.
    The idea is that JSONParser is the better API, and JSON and JSONParseResult could be removed.

@madmiraal
Copy link
Contributor Author

Superseded by #44806.

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