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: Add Sqllite json_valid #4465

Merged
merged 4 commits into from
Feb 14, 2025
Merged

Conversation

dhirensr
Copy link
Contributor

Related to #4366

also I couldn't test the functionality fully, and I've asked a question related to testing in #4463

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR and sorry for taking some days to get back with hints what's wrong here.

The implementation is already on a good way, there are only minor nits that needs to be fixed to run the tests + make this good to merge.

@dhirensr
Copy link
Contributor Author

@weiznich : thank you for reviewing. I can make the changes quickly but facing some errors in testing the change. I tried asking the testing mechanism here : #4463

can you please help? it will be helpful for me in next couple of pull requests.

@weiznich
Copy link
Member

I'm sorry that it looks like #4463 was not answered. From my point of view the issues outlined there were addressed with the earlier set of comments here.

There was a new set of errors in the relevant doc test, which was essentially caused by the set of comments above being not 100% complete in what needed to be changed. I've now pushed 6399f89 to fix these issues, as they were really only minimal syntax errors.

I've also rebased the PR and pushed 128ffba to pull in recent changes from the master branch.

Thanks again for contributing ❤️

@weiznich weiznich enabled auto-merge February 14, 2025 09:31
@weiznich weiznich added this pull request to the merge queue Feb 14, 2025
Merged via the queue into diesel-rs:master with commit 6e4ee3d Feb 14, 2025
34 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.

2 participants