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

Add JSON_REPLACE, JSON_ARRAY, and JSON_SET functions #138

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

johnhaley81
Copy link

I saw that #86 was open for a bit and I needed these specific functions so I went ahead and attempted to add them into the codebase.

This PR adds in JSON_REPLACE, JSON_ARRAY, and JSON_SET to the parser. I wrote tests for them as well and tested them locally in my project and they all work as expected.

The only issue I ran into was that I wanted to add the variadic version of JSON_SET but that takes in key/value pairs of arguments at the end which I couldn't figure out how to implement in the code. Maybe I missed something since I'm very new to this codebase?

Please LMK if there are any changes you would like me to make or if there is something that I missed that wasn't caught by the compiler and/or tests.

@johnhaley81 johnhaley81 mentioned this pull request Oct 17, 2024
@tatchi
Copy link
Contributor

tatchi commented Oct 18, 2024

I believe @jongleb is the most appropriate person to review this

@jongleb
Copy link
Collaborator

jongleb commented Oct 18, 2024

@tatchi Thanks for ping, i didn't see it.

@johnhaley81
I had plans to add these after adding a separate json type, and I also wanted to provide the user with the opportunity to define from and to for him by passing some yojson.
But in anycase, I understand that you urgently need these functions and the code looks good, so I will do it later.
Text json is okay for now.

variadic version of JSON_SET

I would say to take a look how coalesce works. But I was waiting for the second time usage of the variadic to implement more general solution. So, if it's okay currently to use only 3 required arguments, let's keep it as is.
depending on how urgently you need these functions and whether you need variadiс right now

@jongleb jongleb self-requested a review October 18, 2024 07:57
@johnhaley81
Copy link
Author

So, if it's okay currently to use only 3 required arguments, let's keep it as is.

@jongleb I think having the non-variadic version of JSON_SET is good for the moment. This is good for me if it's good for you guys.

@johnhaley81 johnhaley81 changed the base branch from master to ahrefs October 23, 2024 05:23
@jongleb jongleb merged commit d08f47b into ygrek:ahrefs Oct 23, 2024
1 check failed
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.

3 participants