#2165 JavascriptFunctions: refactor to support objects as parameter#2492
#2165 JavascriptFunctions: refactor to support objects as parameter#2492
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the robustness and clarity of JavaScript function support within ArcadeDB. It focuses on improving how JavaScript functions handle diverse input types like JSON objects and strings, refines the internal mechanism for defining these functions, and boosts overall code quality through better organization and comprehensive testing.
Highlights
- JavaScript Function Input Handling: Enhanced support for passing JSON objects and strings as parameters to JavaScript functions, including new tests and removal of unnecessary string quoting logic.
- JavaScript Function Definition Refactoring: Improved the construction of JavaScript function declarations by switching from string concatenation to StringBuilder for better performance and safety.
- Code Clarity and Test Coverage: Cleaned up import statements across several files and added comprehensive tests for various JavaScript function input scenarios, improving overall code readability and robustness.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively enhances JavaScript function support for object and string parameters. The changes are well-tested with new additions to both e2e and engine tests. The refactoring in JavascriptFunctionDefinition to use StringBuilder is a good improvement. I have one suggestion regarding test isolation in JsFunctionsTest.java to ensure the new tests clean up after themselves, preventing potential side effects.
| database.command("sql", """ | ||
| DEFINE FUNCTION Test.objectComparison "return a.foo == 'bar'" PARAMETERS [a] LANGUAGE js; | ||
| """); | ||
|
|
||
| ResultSet resultSet = database.query("sql", """ | ||
| SELECT `Test.objectComparison`('{"foo":"bar"}') as matchRating; | ||
| """); | ||
|
|
||
| assertThat(resultSet.next().<Boolean>getProperty("matchRating")).isTrue(); |
There was a problem hiding this comment.
This test, along with testStringObjectAsInput, defines a new function but does not clean it up after execution. This breaks test isolation and can lead to side effects in other tests. To ensure each test is self-contained and cleans up its resources, you should use a try...finally block to delete the function after the test completes.
| database.command("sql", """ | |
| DEFINE FUNCTION Test.objectComparison "return a.foo == 'bar'" PARAMETERS [a] LANGUAGE js; | |
| """); | |
| ResultSet resultSet = database.query("sql", """ | |
| SELECT `Test.objectComparison`('{"foo":"bar"}') as matchRating; | |
| """); | |
| assertThat(resultSet.next().<Boolean>getProperty("matchRating")).isTrue(); | |
| database.command("sql", """ | |
| DEFINE FUNCTION Test.objectComparison "return a.foo == 'bar'" PARAMETERS [a] LANGUAGE js; | |
| """); | |
| try { | |
| ResultSet resultSet = database.query("sql", """ | |
| SELECT `Test.objectComparison`('{"foo":"bar"}') as matchRating; | |
| """); | |
| assertThat(resultSet.next().<Boolean>getProperty("matchRating")).isTrue(); | |
| } finally { | |
| database.command("sql", "DELETE FUNCTION Test.objectComparison"); | |
| } |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
…add tests for objects and string
94f74d6 to
0dc4122
Compare
This pull request enhances support for JavaScript functions in ArcadeDB, particularly in handling JSON and string inputs, and improves code clarity and test coverage. The most significant changes involve adding new tests for JavaScript function input scenarios, refining how JavaScript function definitions are constructed and executed, and cleaning up imports for better readability.
JavaScript function input handling:
PolyglotFunctionTest.javaandJsFunctionsTest.java. [1] [2]JavaScript function definition construction:
JavascriptFunctionDefinition.javato useStringBuilderfor safer and more efficient string concatenation.Code clarity and organization:
JavascriptFunctionDefinition.javaand test files for better readability and maintainability. [1] [2] [3]These changes collectively improve the robustness and clarity of JavaScript function support in ArcadeDB.