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

Implement json_quote #763

Merged
merged 9 commits into from
Feb 7, 2025
Merged

Conversation

pedrocarlo
Copy link
Contributor

Hi! This is my first PR on the project, so I apologize if I did not follow a convention from the project.

#127
This PR implements json_quote as specified in their source: https://www.sqlite.org/json1.html#jquote. It follows the internal doc guidelines for implementing functions. Most tests were added from sqlite test suite for json_quote, while some others were added by me. Sqlite test suite for json_quote depends on json_valid to test for correct escape control characters, so that specific test at the moment cannot be done the same way.

@pedrocarlo
Copy link
Contributor Author

Maybe it would be better to review first my refactor of json serialization and then later get back to this PR. #771

penberg added a commit that referenced this pull request Jan 29, 2025
…ty printing' from Pedro Muniz

Json serialization logic was pulled from serde_json. Google's json5
serialization code was not flexible enough to allow for pretty printing
json, so I believe that the formatter design is a good layer to abstract
this logic. This refactor will trivially enable the implementation of
json_pretty function from sqlite. My other PR for json_quote, #763,
depends a tiny bit on a helper utility from the previous serialization
implementation. If this PR is considered first, I will change the code
in my other PR to account for this.

Reviewed-by: Diego Reis (@diegoreis42)
Reviewed-by: Kacper Madej (@madejejej)

Closes #771
@jussisaurio
Copy link
Collaborator

Hey @pedrocarlo , json_valid support was merged in #789 so maybe this could be rebased?

@pedrocarlo
Copy link
Contributor Author

pedrocarlo commented Feb 2, 2025

Hey @jussisaurio. Yeah, I need to do that. I was waiting to see if my other refactor was going to get merged before editing this PR again. It got merged, but I still haven't gotten back to this PR. Should be able to do a rebase and some other corrections soon here. This PR is already too many commits behind from main.

@pedrocarlo
Copy link
Contributor Author

@jussisaurio just did the rebase. I think there should be no more merge conflicts.

core/json/mod.rs Outdated Show resolved Hide resolved
core/json/mod.rs Outdated Show resolved Hide resolved
@penberg
Copy link
Collaborator

penberg commented Feb 5, 2025

Hey @pedrocarlo, I think 56d401f caused some conflicts here. Can you please rebase and let's get this merged?

Copy link
Contributor

@sonhmai sonhmai left a comment

Choose a reason for hiding this comment

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

good by me. thanks!

core/json/mod.rs Outdated Show resolved Hide resolved
@pedrocarlo
Copy link
Contributor Author

Hey @pedrocarlo, I think 56d401f caused some conflicts here. Can you please rebase and let's get this merged?

Yeah second rebase I had to do already haha. I will do it today.

@pedrocarlo
Copy link
Contributor Author

@penberg just rebased it

@penberg penberg merged commit 8c0c967 into tursodatabase:main Feb 7, 2025
27 checks passed
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.

4 participants