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

feat(p/json): Update JSON package with new validators and functions #1869

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Apr 1, 2024

Description

This pull request updates the JSON package with new validators and functions. It includes the following changes:

New Feature

  • Added Validate function to apply a list of validators to a node.
  • Added Filter function to filter children of an object or array node based on a predicate.
  • Added Update to update the value or add a new key-value pair to an object node.
  • Added UpdateValue function to update the value of a node.

FIX

  • Updated the README and invalid explanations in comment

@notJoon notJoon requested review from a team as code owners April 1, 2024 09:28
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Apr 1, 2024
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.84%. Comparing base (6760265) to head (78b2d82).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1869      +/-   ##
==========================================
- Coverage   47.54%   44.84%   -2.70%     
==========================================
  Files         388      459      +71     
  Lines       61242    67588    +6346     
==========================================
+ Hits        29117    30311    +1194     
- Misses      29686    34736    +5050     
- Partials     2439     2541     +102     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Hey @notJoon! Thanks for improving the package. I left some comments that might not need to be addressed in this PR. Take a look at them if you can and leave your thoughts.

I will play around with the package a bit to test out the performance.

@@ -96,29 +101,33 @@ Encoding (or Marshaling) is the functionality that converts JSON data represente

> ⚠️ Caution: Converting a large `Node` type into a JSON string may _impact performance_. or might be cause _unexpected behavior_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: or might be cause _unexpected behavior_.

@@ -71,23 +71,28 @@ Decoding (or Unmarshaling) is the functionality that converts an input byte slic

The converted `Node` type allows you to modify the JSON data or search and extract data that meets specific conditions.

Below is an example of converting a `Node` type into a JSON string, and retrieving all the keys in the JSON data with `json.UniqueKeyLists()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you break lines in .md files at 80 chars for easier readability in raw mode ;)

@@ -71,23 +71,28 @@ Decoding (or Unmarshaling) is the functionality that converts an input byte slic

The converted `Node` type allows you to modify the JSON data or search and extract data that meets specific conditions.

Below is an example of converting a `Node` type into a JSON string, and retrieving all the keys in the JSON data with `json.UniqueKeyLists()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a little bit confusing that you're calling a whole JSON string a Node, and then you have each subfield as a Node as well. Was there a specific reason to not call the fields inside the JSON string Fields? Like, FieldString or StringField? WDYT?

@@ -71,23 +71,28 @@ Decoding (or Unmarshaling) is the functionality that converts an input byte slic

The converted `Node` type allows you to modify the JSON data or search and extract data that meets specific conditions.

Below is an example of converting a `Node` type into a JSON string, and retrieving all the keys in the JSON data with `json.UniqueKeyLists()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any metrics on the package? What about adding a few classic test cases to see the performance (with gno test . -print-runtime-metrics)? I think this would also turn into a great piece of code for VM stress-testing later down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants