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

Let JavaScript parse JSON and write JSON ... #3987

Merged
merged 49 commits into from
Dec 20, 2022
Merged

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Dec 15, 2022

Pull Request Description

Use JavaScript to parse and serialise to JSON. Parses to native Enso object.

  • .to_json now returns a Text of the JSON.
  • Json methods now parse, stringify and from_pairs.
  • New JSON_Object representing a JavaScript Object.
  • .to_js_object allows for types to custom serialize. Returning a JS_Object.
  • Default JSON format for Atom now has a type and constructor property (or method to call for as needed to deserialise).
  • Removed .into support for now.
  • Added JSON File Format and SPI to allow Data.read to work.
  • Added Data.fetch API for easy Web download.
  • Default visualization for JS Object trunctes, and made Vector default truncate children too.

Fixes defect where types with no constructor crashed on to_json (e.g. Matching_Mode.Last.to_json.
Adjusted default visualisation for Vector, so it doesn't serialise an array of arrays forever.
Likewise, JS_Object default visualisation is truncated to a small subset.

New convention:

  • .get returns Nothing if a key or index is not present. Takes an other argument allowing control of default.
  • .at error if key or index is not present.
  • Nothing gains a get method allowing for easy propagation.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@jdunkerley jdunkerley force-pushed the wip/jd/javascript-json branch 4 times, most recently from 32c1ba3 to 0623886 Compare December 19, 2022 08:45
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

first batch of comments

CHANGELOG.md Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data.enso Outdated Show resolved Hide resolved
Comment on lines +50 to +47
Number -> JS_Object.from_pairs [["type", "Number"]]
Integer -> JS_Object.from_pairs [["type", "Integer"]]
Decimal -> JS_Object.from_pairs [["type", "Decimal"]]
Copy link
Member

Choose a reason for hiding this comment

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

Good catch that these need to be accounted for. Does it work though? I'm not sure how it plays with self tbh.

Regardless if it works (well if not - then even more so) - this really feels weird to look at - ofc not something to do for now, but I think we should figure out how to distinguish defining such methods for the instance and their static variants. My intuition would see them as rather separate definitions, instead of having to do such matches.

@hubertp @kustosz as you've been close to the statics - any thoughts? Is it even possible to have a static and non-static variant of a method with same name? I'm worried it may conflict with the Type.instance_method instance_ref args call pattern, so I'm not sure if it is even possible. Still seems that from the 'user' (in this context: library developer) perspective, we need to be able to define some methods both on instances and on the type itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these work. I agree that it is not a great pattern but is needed in the current set up.

b.append ["type", "Locale"]
if self.language.is_nothing.not then b.append ["language", self.language]
b.append ["constructor", "new"]
Copy link
Member

Choose a reason for hiding this comment

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

Why is the constructor a method name? Seems surprising.
Do we even need the constructor here? I guess it makes sense to be consistent with Atom serialization, but we put a method name instead of true constructor so we are not truly consistent anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to make it, so we had the information to deserialise.
Originally had fully qualified type names but decided against it for this version.

@jdunkerley jdunkerley marked this pull request as ready for review December 19, 2022 14:39
_ -> value.to_json

json.to_text
"" + json
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is "" still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

JavaScript strings cause an issue with the visualisation. The "" + makes it an Enso Text and then it works,

@jdunkerley jdunkerley added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Dec 19, 2022
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Dec 20, 2022
@mergify mergify bot merged commit ace459e into develop Dec 20, 2022
@mergify mergify bot deleted the wip/jd/javascript-json branch December 20, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants