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

IsValidJSON() descriptor #1325

Merged

Conversation

gagan-bhullar-tech
Copy link
Contributor

Added a descriptor for IsValidJSON()

PR closes #1316

@emeli-dral
Copy link
Contributor

Hi @gagan-bhullar-tech ,
thank you for your contribution!
It was pleasure to review it, from the technical point of view everything looks really good 👍

I only have two requests:
1/ one regarding the naming. Could you please rename the descriptor from IsJSONValid() to IsValidJSON(). We prefer to maintain consistency across all descriptors by using statement formats rather than questions for the naming convention.

2/ and one more regarding the dosc. Could you please add the descriptor to the list of all descriptors under Text Evals heading on the documentation page.

@gagan-bhullar-tech
Copy link
Contributor Author

@emeli-dral updated PR. Please review.

@emeli-dral
Copy link
Contributor

@gagan-bhullar-tech
Thanks for the update! It looks great 👍
Let's fix tests and we merge it.
Please take a look at error messages (I believe messages are the same in all tests), it is something minor with class paths registration.

@gagan-bhullar-tech
Copy link
Contributor Author

@emeli-dral updated PR

@gagan-bhullar-tech
Copy link
Contributor Author

@emeli-dral not sure why that single test is failing. This does not look like related to my code.

@emeli-dral emeli-dral added the hacktoberfest Accepted contributions will count towards your hacktoberfest PRs label Oct 16, 2024
@emeli-dral
Copy link
Contributor

Hi @gagan-bhullar-tech,
All tests have successfully passed, and we’re ready to merge your PR!
Thank you for your hard work!

@emeli-dral emeli-dral merged commit 9f2fbb8 into evidentlyai:main Oct 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Accepted contributions will count towards your hacktoberfest PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new IsValidJSON() descriptor to Evidently
2 participants