Skip to content

Conversation

@torch2424
Copy link
Collaborator

Yo! 👋

So firstly, my apologies for the large PR 🙃

What had happened was, my plan was to just update the README, but, when I tried using the API the way the README laid it out. It didn't work. And what I assumed happened was that the README must have gotten super out of date with the actual implementation, and thus no longer worked. So I ended up just cleaning up the JSON namespace. And API. And since the PR was already huge by then, I was like eh, the last thing I wanted to do was add reference docs using Typedoc, so, let's just do it in this PR 🙃

Anyways! What this PR does:

  • Removes the JSON Namespace, and instead, uses the ES module nampespacing pattern (Which I actually fixed in the compiler 🎉 💪 )
  • Implements a .valueOf function on our JSON types, to get the underlying primitive type.
  • Implements the getX functions on the JSON.Obj.
  • Impelements the .isX on the parent JSON.Value class.
  • Cleans up the README and fixes up all the API info
  • Creates a test to make sure the Usage in the README works.
  • Installs Typedoc to generate reference API documentation
  • Adds some scripts to generate and "deploy" docs so that they match the latest code
  • Adds generating docs to the Travis (It's super helpful for catching bugs! 😄 )

And to mak the review a lot easier, I'd highly suggest just ignoring everything in docs/ as that is all generated 😄

Let me know what you think! Thanks! cc @willemneal

Screenshot from 2021-02-05 17-58-05
Screenshot from 2021-02-05 17-57-55

assembly/JSON.ts Outdated
const objs: string[] = [];
for (let i: i32 = 0; i < this.keys.length; i++) {
objs.push(
'"' + this.keys[i] + '":' + this._obj.get(this.keys[i]).toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any tests for this? Given the error above in value's toString it seems like since the value isn't cast to the subclass it should fail.

Copy link
Contributor

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

Thanks this looks great! My one request is a simple test for toString for each class.

@torch2424
Copy link
Collaborator Author

@willemneal Made your requested changes! This should now be good to go! 😄 🎉

Copy link
Contributor

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@willemneal willemneal merged commit c6508f5 into near:master Feb 10, 2021
@torch2424 torch2424 deleted the torch2424/readme-cleanup branch February 10, 2021 17:47
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.

2 participants