Feat operators #1225
Feat operators #1225
Conversation
WalkthroughAdds Operator and Condition templates and file mappings to many SDK generators (Android, Apple/Swift, Dart/Flutter, Deno, DotNet, Go, Kotlin, Node, PHP, Python, React Native, Ruby, Swift, Web). Introduces language-specific Operator implementation templates (and some corresponding tests), updates package/module exports/imports/parts to include Operator/Condition, and extends autogenerated test fixtures and language tests to exercise the new Operator helpers. A new protected constant OPERATOR_HELPER_RESPONSES was added to tests/Base.php. All changes are additive. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
5-10: Same serializer considerations as Kotlin filePlease apply the same verification re: empty lists and nulls in arrays.
🧹 Nitpick comments (23)
templates/ruby/lib/container/operator.rb.twig (2)
28-28: Useclass << selffor singleton methods.
class << Operatorworks but is unconventional and can confuse constant scope; preferclass << self.- class << Operator + class << self
17-22: Avoid reliance onHash#compactor confirm Ruby minimum version.If supported Ruby targets may be < 2.4/2.5,
Hash#compactmight be unavailable. Safer inline alternative:- { - method: @method, - values: @values - }.compact.to_json(*args) + payload = { method: @method } + payload[:values] = @values unless @values.nil? + payload.to_json(*args)templates/python/package/operator.py.twig (1)
4-10: Initialize self.values explicitlyMinor clarity: set self.values = None before the conditional, so the attribute always exists.
class Operator(): def __init__(self, method, values=None): self.method = method - - if values is not None: - self.values = values if isinstance(values, list) else [values] + self.values = None + if values is not None: + self.values = values if isinstance(values, list) else [values]templates/php/src/Operator.php.twig (2)
26-32: Don’t drop empty arrays; only drop nullarray_filter() without a callback removes empty arrays, so "values": [] gets omitted (toggle/arrayUnique/dateSetNow). Keep empty arrays to match other SDKs.
- public function jsonSerialize(): mixed - { - return array_filter([ - 'method' => $this->method, - 'values' => $this->values, - ]); - } + public function jsonSerialize(): mixed + { + // Keep empty arrays; drop only nulls + return array_filter([ + 'method' => $this->method, + 'values' => $this->values, + ], fn($v) => $v !== null); + }
21-24: Safer stringificationGuard against json_encode() returning false and use unescaped options for consistency.
- public function __toString(): string - { - return json_encode($this); - } + public function __toString(): string + { + $json = json_encode($this, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + return $json === false ? '' : $json; + }templates/node/src/operator.ts.twig (1)
1-3: Simplify typesReduce redundancy; use unknown for flexibility without “any”.
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; -export type OperatorValues = OperatorValuesSingle | OperatorValuesList; +export type OperatorValuesList = unknown[]; +export type OperatorValues = unknown | unknown[];templates/deno/src/operator.ts.twig (4)
1-3: Tighten types:any[]makes the union redundant.
export type OperatorValuesList = string[] | number[] | boolean[] | any[]collapses toany[]. PreferReadonlyArray<unknown>for stronger typing and consistency.Apply:
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; -export type OperatorValues = OperatorValuesSingle | OperatorValuesList; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = ReadonlyArray<unknown>; +export type OperatorValues = OperatorValuesSingle | OperatorValuesList;
9-11: Prefer immutability for fields.Mark
methodandvaluesas readonly and simplify normalization.-export class Operator { - method: string; - values: OperatorValuesList | undefined; +export class Operator { + readonly method: string; + readonly values?: OperatorValuesList; @@ - if (values !== undefined) { - if (Array.isArray(values)) { - this.values = values; - } else { - this.values = [values] as OperatorValuesList; - } - } + if (values !== undefined) { + this.values = Array.isArray(values) ? values : [values]; + }Also applies to: 24-31
199-201: Avoid serializing[condition, null]whenvalueis omitted.
JSON.stringifyturnsundefinedwithin arrays intonull. Build the array conditionally.- static arrayFilter = (condition: string, value?: any): string => - new Operator("arrayFilter", [condition, value]).toString(); + static arrayFilter = (condition: string, value?: any): string => { + const values = value === undefined ? [condition] : [condition, value]; + return new Operator("arrayFilter", values).toString(); + };Please confirm server-side expects the second argument to be omitted (not null) when unused.
1-254: De-duplicate TS templates via a Twig partial.Deno/Web/React‑Native implementations are identical. Extract a single
operator.ts.twigpartial to prevent drift.templates/react-native/src/operator.ts.twig (4)
1-3: Tighten types: replace union withReadonlyArray<unknown>.-export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +export type OperatorValuesList = ReadonlyArray<unknown>;
9-11: Make fields readonly and simplify normalization.- method: string; - values: OperatorValuesList | undefined; + readonly method: string; + readonly values?: OperatorValuesList; @@ - if (values !== undefined) { - if (Array.isArray(values)) { - this.values = values; - } else { - this.values = [values] as OperatorValuesList; - } - } + if (values !== undefined) { + this.values = Array.isArray(values) ? values : [values]; + }Also applies to: 24-31
199-201: Drop null second arg inarrayFilter.- static arrayFilter = (condition: string, value?: any): string => - new Operator("arrayFilter", [condition, value]).toString(); + static arrayFilter = (condition: string, value?: any): string => { + const values = value === undefined ? [condition] : [condition, value]; + return new Operator("arrayFilter", values).toString(); + };
1-254: Extract shared TS logic into a Twig partial to avoid drift across targets.templates/dart/lib/operator.dart.twig (2)
25-41: Align defaults with other languages (value = 1).TS/C# default
increment/decrementto 1. Mirror for Dart to keep SDK parity.- static String increment(num value, [num? max]) { + static String increment([num value = 1, num? max]) { @@ - static String decrement(num value, [num? min]) { + static String decrement([num value = 1, num? min]) {Confirm this change won’t break existing generator expectations/tests.
102-105: Avoid[condition, null]when optionalvalueis not provided.- static String arrayFilter(String condition, [dynamic value]) => - Operator._('arrayFilter', [condition, value]).toString(); + static String arrayFilter(String condition, [dynamic value]) { + final values = <dynamic>[condition]; + if (value != null) values.add(value); + return Operator._('arrayFilter', values).toString(); + }Ensure tests expect omission (not null) for the second argument.
templates/web/src/operator.ts.twig (4)
1-3: Simplify toReadonlyArray<unknown>; remove redundant union.-export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +export type OperatorValuesList = ReadonlyArray<unknown>;
9-11: Readonly fields and simpler normalization.- method: string; - values: OperatorValuesList | undefined; + readonly method: string; + readonly values?: OperatorValuesList; @@ - if (values !== undefined) { - if (Array.isArray(values)) { - this.values = values; - } else { - this.values = [values] as OperatorValuesList; - } - } + if (values !== undefined) { + this.values = Array.isArray(values) ? values : [values]; + }Also applies to: 24-31
199-201: ConditionalarrayFiltersecond parameter to avoid nulls.- static arrayFilter = (condition: string, value?: any): string => - new Operator("arrayFilter", [condition, value]).toString(); + static arrayFilter = (condition: string, value?: any): string => { + const values = value === undefined ? [condition] : [condition, value]; + return new Operator("arrayFilter", values).toString(); + };
1-254: Extract a shared TS Twig partial for Operator to keep Deno/Web/RN in sync.templates/dotnet/Package/Operator.cs.twig (2)
13-15: Ensurevaluesnulls are not serialized.For parity with other SDKs, omit
valueswhen null. Add attribute guard.- [JsonPropertyName("values")] - public List<object>? Values { get; set; } + [JsonPropertyName("values")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public List<object>? Values { get; set; }If
Client.SerializerOptionsalready ignores nulls, this is optional but makes it explicit.
133-136: Avoid[condition, null]whenvalueis not provided.- public static string ArrayFilter(string condition, object? value = null) - { - return new Operator("arrayFilter", new List<object> { condition, value! }).ToString(); - } + public static string ArrayFilter(string condition, object? value = null) + { + var values = new List<object> { condition }; + if (value is not null) values.Add(value); + return new Operator("arrayFilter", values).ToString(); + }Confirm the API expects omission, not JSON null, for the optional arg.
templates/go/operator.go.twig (1)
117-123: Consider validating the index parameter.
ArrayInsertaccepts anindexparameter but doesn't validate if it's negative or otherwise invalid. While server-side validation may handle this, an early client-side check could improve developer experience.func ArrayInsert(index int, value interface{}) string { + if index < 0 { + // Consider: panic, return error, or let server validate + } values := []interface{}{index, value} return parseOperator(operatorOptions{ Method: "arrayInsert", Values: &values, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
src/SDK/Language/Android.php(1 hunks)src/SDK/Language/Apple.php(1 hunks)src/SDK/Language/Dart.php(2 hunks)src/SDK/Language/Deno.php(1 hunks)src/SDK/Language/DotNet.php(1 hunks)src/SDK/Language/Flutter.php(2 hunks)src/SDK/Language/Go.php(1 hunks)src/SDK/Language/Kotlin.php(1 hunks)src/SDK/Language/Node.php(1 hunks)src/SDK/Language/PHP.php(1 hunks)src/SDK/Language/Python.php(1 hunks)src/SDK/Language/ReactNative.php(1 hunks)src/SDK/Language/Ruby.php(1 hunks)src/SDK/Language/Swift.php(1 hunks)src/SDK/Language/Web.php(1 hunks)templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/dart/test/operator_test.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/deno/test/operator.test.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/php/tests/OperatorTest.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)tests/Android14Java11Test.php(1 hunks)tests/Android14Java17Test.php(1 hunks)tests/Android14Java8Test.php(1 hunks)tests/Android5Java17Test.php(1 hunks)tests/AppleSwift56Test.php(1 hunks)tests/Base.php(1 hunks)tests/DartBetaTest.php(1 hunks)tests/DartStableTest.php(1 hunks)tests/Deno1193Test.php(1 hunks)tests/Deno1303Test.php(1 hunks)tests/DotNet60Test.php(1 hunks)tests/DotNet80Test.php(1 hunks)tests/DotNet90Test.php(1 hunks)tests/FlutterBetaTest.php(1 hunks)tests/FlutterStableTest.php(1 hunks)tests/Go112Test.php(1 hunks)tests/Go118Test.php(1 hunks)tests/KotlinJava11Test.php(1 hunks)tests/KotlinJava17Test.php(1 hunks)tests/KotlinJava8Test.php(1 hunks)tests/Node16Test.php(1 hunks)tests/Node18Test.php(1 hunks)tests/Node20Test.php(1 hunks)tests/PHP80Test.php(1 hunks)tests/PHP83Test.php(1 hunks)tests/Python310Test.php(1 hunks)tests/Python311Test.php(1 hunks)tests/Python312Test.php(1 hunks)tests/Python313Test.php(1 hunks)tests/Python39Test.php(1 hunks)tests/Ruby27Test.php(1 hunks)tests/Ruby30Test.php(1 hunks)tests/Ruby31Test.php(1 hunks)tests/Swift56Test.php(1 hunks)tests/WebChromiumTest.php(1 hunks)tests/WebNodeTest.php(1 hunks)tests/languages/android/Tests.kt(2 hunks)tests/languages/apple/Tests.swift(1 hunks)tests/languages/dart/tests.dart(1 hunks)tests/languages/deno/tests.ts(2 hunks)tests/languages/dotnet/Tests.cs(1 hunks)tests/languages/flutter/tests.dart(1 hunks)tests/languages/go/tests.go(3 hunks)tests/languages/kotlin/Tests.kt(2 hunks)tests/languages/node/test.js(2 hunks)tests/languages/php/test.php(3 hunks)tests/languages/python/tests.py(2 hunks)tests/languages/ruby/tests.rb(1 hunks)tests/languages/swift/Tests.swift(1 hunks)tests/languages/web/index.html(1 hunks)tests/languages/web/node.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (38)
tests/AppleSwift56Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Android14Java8Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Ruby31Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/KotlinJava17Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Node16Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/PHP80Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/PHP83Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/KotlinJava8Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Go112Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Deno1303Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Ruby30Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Go118Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Swift56Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/DotNet80Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/DartStableTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/FlutterBetaTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/WebNodeTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/Python310Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Node18Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Ruby27Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Android5Java17Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/DotNet90Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/languages/web/node.js (1)
tests/languages/node/test.js (3)
require(1-12)require(13-13)require(14-14)
tests/WebChromiumTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/languages/android/Tests.kt (1)
tests/languages/kotlin/Tests.kt (1)
writeToFile(271-274)
tests/Node20Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Python311Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/KotlinJava11Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Android14Java17Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Python39Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Python313Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/FlutterStableTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/languages/kotlin/Tests.kt (1)
tests/languages/android/Tests.kt (1)
writeToFile(304-307)
tests/DartBetaTest.php (1)
tests/Base.php (1)
Base(17-326)
tests/Android14Java11Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Python312Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/Deno1193Test.php (1)
tests/Base.php (1)
Base(17-326)
tests/DotNet60Test.php (1)
tests/Base.php (1)
Base(17-326)
🪛 detekt (1.23.8)
tests/languages/android/Tests.kt
[warning] 279-279: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (76)
tests/Go112Test.php (1)
30-31: LGTM: test now validates operator helper outputs.Including
OPERATOR_HELPER_RESPONSESkeeps Go tests aligned with new Operator helpers.tests/DotNet60Test.php (1)
32-34: LGTM: added Operator expectations for .NET 6 suite.Spread of
OPERATOR_HELPER_RESPONSESmatches Base fixtures.tests/Python39Test.php (1)
32-34: LGTM: Python 3.9 tests include operator fixtures.Keeps parity with other language test suites.
tests/PHP80Test.php (1)
28-29: LGTM! Consistent integration of operator helper responses.The addition of
OPERATOR_HELPER_RESPONSESto the expected output follows the established pattern used for Query, Permission, and ID helpers. This change enables validation of the new Operator functionality in PHP SDK tests.tests/Go118Test.php (1)
30-30: LGTM! Go SDK test expectations updated correctly.The integration of
OPERATOR_HELPER_RESPONSESis consistent with the broader operator feature rollout across all SDK languages.src/SDK/Language/Flutter.php (2)
88-92: LGTM! Operator library file registration follows established conventions.The operator library file is registered consistently with other helper utilities (Query, Permission, ID, Role). The placement in the file structure is logical and maintains the existing organization pattern.
283-287: LGTM! Operator test file properly registered.The test file registration mirrors the pattern used for other helper tests (id_test.dart, permission_test.dart, query_test.dart), ensuring comprehensive test coverage for the new Operator functionality.
tests/PHP83Test.php (1)
28-29: LGTM! PHP 8.3 test expectations updated consistently.The addition aligns with the operator helper integration across all PHP SDK test variants.
tests/DotNet80Test.php (1)
32-33: LGTM! .NET SDK test expectations properly extended.The operator helper responses are correctly integrated following the established test pattern.
tests/Ruby27Test.php (1)
30-31: LGTM! Ruby SDK test expectations updated correctly.Consistent integration of operator helper validation for the Ruby SDK.
tests/FlutterStableTest.php (1)
33-34: LGTM! Flutter test expectations properly updated.The operator helper responses are correctly added to validate the Flutter SDK's operator functionality.
tests/Android14Java17Test.php (1)
34-35: LGTM! Android SDK test expectations properly extended.The addition of
OPERATOR_HELPER_RESPONSEScompletes the consistent operator helper integration across all language SDKs.tests/KotlinJava11Test.php (1)
33-34: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESto the expected output follows the established pattern and correctly uses the spread operator with proper syntax.tests/Python310Test.php (1)
32-33: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/Ruby30Test.php (1)
30-31: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/Android5Java17Test.php (1)
34-35: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/Python312Test.php (1)
32-33: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/Ruby31Test.php (1)
30-31: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/DartBetaTest.php (1)
32-33: LGTM! Test expectations extended correctly.The addition of
OPERATOR_HELPER_RESPONSESfollows the established pattern and is consistent with other test suite updates.tests/Node20Test.php (1)
36-37: LGTM! Consistent addition of operator test expectations.The addition of
OPERATOR_HELPER_RESPONSESto the expected output follows the established pattern and correctly extends the test suite to validate operator functionality.tests/Android14Java8Test.php (1)
35-36: LGTM! Consistent operator test integration.The operator helper responses are correctly added to the expected output, maintaining consistency with other test suites.
tests/WebNodeTest.php (1)
37-38: LGTM! Operator test expectations added correctly.The addition maintains consistency with the pattern established across all test suites.
tests/Swift56Test.php (1)
32-33: LGTM! Consistent operator test addition.The operator helper responses are correctly integrated into the Swift test expectations.
src/SDK/Language/Deno.php (2)
76-80: LGTM! Operator test file generation added correctly.The test file generation entry properly mirrors the pattern established by query tests. The verification script above will confirm both templates exist.
66-70: Template exists and configuration is correct.Verification confirms that
deno/src/operator.ts.twigexists at the expected location. The operator source file generation is correctly configured in the Deno language driver.tests/KotlinJava17Test.php (1)
33-34: LGTM! Operator test expectations integrated consistently.The addition of operator helper responses follows the established pattern across all test suites.
src/SDK/Language/Python.php (1)
163-167: Template file verified—no further action needed.The operator.py.twig template file exists and is correctly referenced. The operator file generation entry is properly structured and placed logically after query.py generation.
src/SDK/Language/Ruby.php (1)
150-154: Template verified and correctly implemented.The Ruby operator template file exists at
templates/ruby/lib/container/operator.rb.twigand the file generation entry is properly structured with correct path reference.tests/Node18Test.php (1)
36-37: LGTM!The addition of
OPERATOR_HELPER_RESPONSESto the expected output array correctly extends the test to validate operator helper functionality.tests/Deno1193Test.php (1)
28-29: LGTM!The operator helper responses are correctly integrated into the expected output array.
tests/Python313Test.php (1)
32-33: LGTM!The operator helper responses are properly added to the expected output for Python 3.13 tests.
tests/Deno1303Test.php (1)
28-29: LGTM!The operator helper responses are correctly integrated for Deno 1.30.3 tests.
tests/Node16Test.php (1)
36-37: LGTM!The operator helper responses are correctly added to the Node 16 test expectations.
tests/Python311Test.php (1)
32-33: LGTM!The operator helper responses are properly integrated for Python 3.11 tests.
src/SDK/Language/Kotlin.php (1)
397-401: LGTM!The Operator.kt file generation entry follows the established pattern for other helper classes (ID, Query, Permission, Role) and is correctly configured.
tests/FlutterBetaTest.php (1)
33-34: LGTM!The operator helper responses are correctly added to the Flutter beta test expectations.
tests/DotNet90Test.php (1)
32-33: LGTM!The addition of OPERATOR_HELPER_RESPONSES to the expected output is correct and consistent with the broader PR changes introducing Operator support across all SDK implementations.
tests/AppleSwift56Test.php (1)
33-34: LGTM!The addition of OPERATOR_HELPER_RESPONSES to the expected output is correctly implemented and aligns with the PR's objective of adding Operator support.
tests/DartStableTest.php (1)
32-33: LGTM!The change correctly adds OPERATOR_HELPER_RESPONSES to the expected test output, maintaining consistency with the PR-wide Operator feature addition.
tests/WebChromiumTest.php (1)
37-38: LGTM!The addition of OPERATOR_HELPER_RESPONSES is correctly implemented and consistent with other Web SDK test configurations in this PR.
src/SDK/Language/Swift.php (1)
175-179: LGTM!The Operator.swift file generation entry is correctly configured and logically positioned among other helper classes (Query, ID). This aligns with the PR's objective of adding Operator support to the Swift SDK.
src/SDK/Language/Apple.php (1)
78-82: LGTM!The Operator.swift file generation entry is correctly configured for the Apple SDK, mirroring the Swift implementation. The placement and structure are consistent with the parent Swift class.
tests/KotlinJava8Test.php (1)
33-34: LGTM!The addition of OPERATOR_HELPER_RESPONSES to the expected output is correctly implemented and consistent with the Kotlin SDK test configuration pattern.
src/SDK/Language/ReactNative.php (1)
68-72: LGTM!The operator.ts file generation entry is consistent with the existing file mapping patterns and properly positioned alongside other helper utilities (query.ts, id.ts, etc.).
src/SDK/Language/Go.php (1)
101-105: LGTM!The operator.go file generation entry is properly formatted and consistent with the existing Go file mapping patterns.
tests/Base.php (1)
166-192: LGTM!The OPERATOR_HELPER_RESPONSES constant is properly formatted and follows the same pattern as the existing helper response constants (QUERY_HELPER_RESPONSES, PERMISSION_HELPER_RESPONSES, ID_HELPER_RESPONSES).
templates/deno/test/operator.test.ts.twig (1)
1-130: LGTM!The Deno operator test template is well-structured and comprehensive. All test invocations correctly match the expected OPERATOR_HELPER_RESPONSES defined in tests/Base.php. The tests properly verify that the toString() serialization outputs the correct JSON format for all operator methods.
tests/languages/dotnet/Tests.cs (1)
228-254: LGTM! Comprehensive Operator test coverage.The Operator helper tests are well-structured and cover a comprehensive set of operations including arithmetic, array manipulation, string operations, boolean toggle, and date utilities. The test patterns are consistent with the existing Query, Permission, Role, and ID helper tests in this file.
tests/languages/go/tests.go (3)
12-12: LGTM! Operator import added correctly.The operator package import is properly placed with other helper imports.
148-149: LGTM! Operator tests properly integrated.The testOperatorHelpers() call is correctly positioned after other helper tests, maintaining consistency with the test flow.
282-309: LGTM! Comprehensive Operator test implementation.The testOperatorHelpers() function provides thorough coverage of all Operator operations using Go-appropriate syntax, including the use of
[]interface{}for array parameters.src/SDK/Language/Node.php (1)
205-209: LGTM! Operator file generation entry properly configured.The new file entry for
operator.tsfollows the established pattern for other helper files (query, id, permission, role) and is correctly positioned in the file generation list.tests/languages/web/node.js (2)
1-1: LGTM! Operator import added correctly.The Operator class is properly added to the destructured imports from the SDK module.
253-279: LGTM! Comprehensive Operator test coverage.The Operator helper tests are well-implemented with JavaScript-appropriate syntax and provide thorough coverage of all operator operations. The tests are correctly positioned in the test flow.
src/SDK/Language/Android.php (1)
128-132: LGTM! Operator.kt generation entry properly configured.The new file entry for
Operator.ktfollows the established pattern for other Android library helper files and is correctly positioned after the Query.kt entry.src/SDK/Language/Dart.php (2)
345-349: LGTM! Operator source file entry properly configured.The new file entry for
operator.dartfollows the established pattern for other Dart helper files and is correctly positioned after the query.dart entry.
440-444: LGTM! Operator test file entry properly configured.The new file entry for
operator_test.dartfollows the established pattern for other Dart test files and is correctly positioned after the query_test.dart entry.tests/languages/android/Tests.kt (2)
11-11: LGTM! Operator import added correctly.The Operator import is properly placed with other helper imports.
271-297: LGTM! Comprehensive Operator test implementation.The Operator helper tests are well-implemented with Kotlin-appropriate syntax (using
listOf()for arrays) and provide thorough coverage of all operator operations. The tests properly use thewriteToFile()helper consistent with other tests in this file.tests/languages/apple/Tests.swift (1)
251-277: LGTM! Comprehensive Operator test implementation.The Operator helper tests are well-implemented with Swift-appropriate syntax, including named parameters (
max:,min:) where applicable. The tests provide thorough coverage of all operator operations and are correctly positioned in the test flow.tests/languages/python/tests.py (1)
11-11: LGTM! Operator tests added correctly.The import and test calls align with the expected operator helper responses.
Also applies to: 201-227
templates/dart/test/operator_test.dart.twig (1)
1-160: LGTM! Comprehensive Operator test template.The test suite provides thorough coverage of all Operator methods with proper JSON decoding and assertions.
src/SDK/Language/PHP.php (1)
215-224: LGTM! Operator file generation entries added correctly.The new entries follow the same pattern as existing helper classes (ID, Query, Role, Permission).
templates/php/tests/OperatorTest.php.twig (1)
1-92: LGTM! Comprehensive PHP Operator test template.The test suite validates all Operator methods with strict JSON output assertions.
tests/Android14Java11Test.php (1)
35-36: LGTM! Expected output updated for Operator tests.The addition of
OPERATOR_HELPER_RESPONSEScorrectly extends the expected test output.tests/languages/php/test.php (2)
10-10: Operator import/use — OKIncludes and namespace use look correct and consistent with generated class.
Also applies to: 24-24
219-245: Add parity checks for null and empty-args casesPlease add invocations that exercise:
- arrayFilter with only the condition (to validate second argument serialized as null).
- zero-arg ops (toggle, arrayUnique, dateSetNow) to confirm whether "values": [] is present across languages.
This helps catch cross-language JSON shape drift early.
templates/python/package/operator.py.twig (1)
11-16: LGTMCompact, deterministic JSON; matches the intended contract.
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (2)
5-10: Verify serializer preserves empty lists and nulls in arraysEnsure {{ sdk.namespace | caseDot }}.extensions.toJson:
- Does not drop empty lists (values: []).
- Preserves null elements inside arrays (e.g., ["equals", null]).
If it filters empties/nulls, tests will diverge from other SDKs.
11-101: API surface — OKFactory methods/readability look consistent and idiomatic.
templates/node/src/operator.ts.twig (1)
38-43: LGTMSerialization is compact; undefined omits the property as intended.
templates/dotnet/Package/Operator.cs.twig (1)
38-41: No issues found;Client.SerializerOptionsis properly configured.Verification confirms
Client.SerializerOptionsexists in the Client class and is configured withDefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, ensuring null values are consistently ignored during serialization. The implementation in Operator.cs.twig is correct.templates/dart/lib/operator.dart.twig (1)
22-24: No action needed —dart:convertis properly imported in the parent library.The operator.dart.twig file is a Dart "part" file and inherits imports from its parent library. The parent library file (package.dart.twig) contains
import 'dart:convert'at line 11, which makesjsonEncode()available to thetoString()method on line 22 of operator.dart.twig. The code is correct as written.templates/go/operator.go.twig (3)
40-101: LGTM: Arithmetic operators follow consistent pattern.The arithmetic operator functions (Increment, Decrement, Multiply, Divide, Modulo, Power) are correctly implemented with consistent handling of optional parameters.
163-185: LGTM: String and boolean operators correctly implemented.The Concat, Replace, and Toggle functions follow the expected patterns and have appropriate type signatures.
187-209: LGTM: Date operators correctly implemented.The date manipulation functions (DateAddDays, DateSubDays, DateSetNow) are properly implemented with appropriate type signatures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
1-104: Identical to Kotlin template - same review applies.This file is identical to
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig. The same review comments apply:
- ✅ Clean operator builder pattern with good API design
⚠️ Division/modulo by zero validation needed (lines 30-38)⚠️ Array index validation should be verified (line 54)- 🔄 Template duplication should be eliminated (see comments on the other file)
Please refer to the review comments on
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twigfor detailed analysis and suggested fixes.
🧹 Nitpick comments (1)
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
1-104: Consider eliminating template duplication.This file is identical to
templates/android/library/src/main/java/io/package/Operator.kt.twig. Having two copies of the same 104-line template creates maintenance overhead and risks divergence.Consider one of these approaches:
Option 1: Extract to a shared template fragment
{# Create templates/shared/operator.kt.twig #} {% include 'shared/operator.kt.twig' %}Option 2: Use template inheritance
{# In both files #} {% extends 'base/operator.kt.twig' %}Option 3: Generate both from a single source
If the build system supports it, generate both target files from a single template definition.This ensures bug fixes and feature additions only need to be applied once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/dart/lib/package.dart.twig(1 hunks)templates/flutter/lib/package.dart.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)
🔇 Additional comments (10)
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (3)
1-10: LGTM! Clean operator builder pattern.The class structure is well-designed with a simple data holder that delegates JSON serialization to the
toJson()extension method. The companion object factory pattern provides a clear API for creating operator descriptors.
30-38: Verify division-by-zero and modulo-by-zero handling.The
divideandmodulomethods acceptdivisorwithout validating that it's non-zero. If these operators are executed without validation elsewhere, this could lead to runtime exceptions.Please confirm whether zero-division validation occurs:
- At the server/API level when these operators are processed
- At runtime before execution
- Or if client-side validation should be added here
If validation doesn't occur elsewhere, consider adding checks:
fun divide(divisor: Number, min: Number? = null): String { + require(divisor.toDouble() != 0.0) { "Divisor cannot be zero" } val values = mutableListOf<Any>(divisor) min?.let { values.add(it) } return Operator("divide", values).toJson() } fun modulo(divisor: Number): String { + require(divisor.toDouble() != 0.0) { "Divisor cannot be zero" } return Operator("modulo", listOf(divisor)).toJson() }
54-56: Verify array index validation.The
arrayInsertmethod accepts anindexwithout validation. Negative indices or out-of-bounds values could cause runtime errors when the operator is applied.Please confirm whether index validation occurs at runtime when the operator is executed, or if client-side validation should be added here to catch negative indices early:
fun arrayInsert(index: Int, value: Any): String { + require(index >= 0) { "Index must be non-negative" } return Operator("arrayInsert", listOf(index, value)).toJson() }templates/flutter/lib/package.dart.twig (1)
33-33: LGTM!The addition of the
operator.dartpart directive cleanly extends the Flutter library composition to include Operator functionality, mirroring the pattern used for other helper components (query, permission, role, id).templates/dart/lib/package.dart.twig (1)
30-30: LGTM!The addition of the
operator.dartpart directive properly integrates the Operator class into the Dart library, maintaining consistency with the existing part structure.templates/dart/lib/operator.dart.twig (5)
1-8: Well-structured class foundation.The private constructor pattern with flexible
valuesparameter (supporting lists, scalars, or null) provides a clean foundation for the factory methods. The use ofdynamicfor values is appropriate here given the variety of types that need to be supported.
10-23: Excellent defensive JSON serialization.The
toJsonmethod includes smart defensive logic at line 16 that ensures values are always serialized as lists while preserving the semantic distinction between empty arrays and null (omitted field). The implementation correctly handles all edge cases.
25-72: Numeric operators correctly implemented.All six numeric operators follow a consistent pattern and properly handle optional boundary parameters (max for increment/multiply/power, min for decrement/divide). The conditional list building logic is correct and the method signatures are semantically appropriate for their respective operations.
74-109: Array operators are well-implemented.All eight array operators are correctly implemented with appropriate signatures. The distinction between methods that accept lists directly (append, prepend, intersect, diff) versus those that construct argument lists (insert, remove, filter) is handled properly. The empty list in
arrayUnique()appears intentional for API compatibility.
111-133: String, boolean, and date operators look good.The final set of operators (concat, replace, toggle, dateAddDays, dateSubDays, dateSetNow) are correctly implemented and follow the same consistent patterns established by the numeric and array operators. The use of empty lists for parameter-less operations maintains consistency across the API surface.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/languages/deno/tests.ts (1)
244-271: Fix parameter mismatches in Operator test calls.The parameter values in multiple Operator test calls don't align with the expected
OPERATOR_HELPER_RESPONSES. The past review comment has already identified all the specific mismatches that need to be corrected, including:
- Line 248:
decrement(5, 0)should bedecrement(3, 0)- Line 251:
modulo(3)should bemodulo(5)- Lines 253-266: Multiple incorrect parameter values for append, prepend, insert, remove, intersect, diff, filter, concat, and replace methods
Please apply the corrections provided in the previous review.
templates/go/operator.go.twig (1)
1-224: Past review comments still apply.The critical issues identified in previous reviews remain valid:
- The unused
toArrayfunction at lines 19-28 should be removed- The ignored
json.Marshalerror at line 47 needs proper handlingtemplates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
58-90: Verify method naming consistency with tests.This Kotlin template defines array operation methods with an "array" prefix (
arrayAppend,arrayPrepend,arrayInsert,arrayRemove,arrayIntersect,arrayDiff,arrayFilter), which is consistent with the Android template but mismatched with the test calls intests/languages/kotlin/Tests.kt(lines 248-259) that use short names without the prefix.This is the same naming issue flagged for the Android template and the test file.
🧹 Nitpick comments (1)
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
74-76: Consider usingnullinstead ofemptyList()for consistency.Methods like
arrayUnique(),toggle(), anddateSetNow()passemptyList()as the values parameter, but sincevaluesis nullable (List<Any>?), passingnullmight be more semantically clear and ensure consistent JSON serialization (omitting the field vs. including an empty array).Example for
arrayUnique():- fun arrayUnique(): String { - return Operator("arrayUnique", emptyList()).toJson() - } + fun arrayUnique(): String { + return Operator("arrayUnique", null).toJson() + }Note: Only apply this change if the API expects the
valuesfield to be omitted rather than present as an empty array. Verify the expected JSON format before making this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/dart/test/operator_test.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/deno/test/operator.test.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/php/tests/OperatorTest.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)tests/languages/android/Tests.kt(2 hunks)tests/languages/apple/Tests.swift(1 hunks)tests/languages/dart/tests.dart(1 hunks)tests/languages/deno/tests.ts(2 hunks)tests/languages/dotnet/Tests.cs(1 hunks)tests/languages/flutter/tests.dart(1 hunks)tests/languages/go/tests.go(3 hunks)tests/languages/kotlin/Tests.kt(2 hunks)tests/languages/node/test.js(2 hunks)tests/languages/php/test.php(3 hunks)tests/languages/python/tests.py(2 hunks)tests/languages/ruby/tests.rb(1 hunks)tests/languages/swift/Tests.swift(1 hunks)tests/languages/web/node.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/languages/dotnet/Tests.cs
- templates/deno/test/operator.test.ts.twig
- tests/languages/flutter/tests.dart
- tests/languages/web/node.js
- tests/languages/apple/Tests.swift
- templates/dart/test/operator_test.dart.twig
- tests/languages/dart/tests.dart
- templates/python/package/operator.py.twig
- templates/react-native/src/operator.ts.twig
- tests/languages/php/test.php
- tests/languages/node/test.js
- tests/languages/go/tests.go
- tests/languages/ruby/tests.rb
- templates/php/src/Operator.php.twig
- templates/php/tests/OperatorTest.php.twig
- tests/languages/android/Tests.kt
- tests/languages/python/tests.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/languages/kotlin/Tests.kt (1)
tests/languages/android/Tests.kt (1)
writeToFile(305-308)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: python (server)
- GitHub Check: kotlin (server)
- GitHub Check: node (server)
- GitHub Check: swift (server)
- GitHub Check: flutter (client)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: react-native (client)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (9)
tests/languages/deno/tests.ts (1)
12-13: LGTM!The Operator and Condition alias declarations follow the established pattern and are properly used in the test suite below.
templates/ruby/lib/container/operator.rb.twig (1)
101-105: LGTM - Past issue resolved.The
array_filtermethod now correctly omits the second element when value is nil, matching the pattern used across other language implementations.templates/dart/lib/operator.dart.twig (1)
1-151: Well-structured implementation with comprehensive documentation.The Dart Operator implementation correctly handles optional parameters and provides clear documentation for all factory methods. The value normalization in
toJsonensures consistent serialization.templates/deno/src/operator.ts.twig (1)
1-269: LGTM - Clean TypeScript implementation.The Deno Operator template provides proper type safety with comprehensive type definitions and well-documented factory methods. The constructor correctly handles value normalization.
templates/node/src/operator.ts.twig (1)
1-269: LGTM - Consistent with Deno implementation.The Node Operator template follows the same solid patterns as the Deno implementation, ensuring consistency across TypeScript-based SDKs.
templates/swift/Sources/Operator.swift.twig (1)
212-218: Consistent with cross-language pattern.The
arrayFilterimplementation matches the behavior across all other language templates (Ruby, Dart, TypeScript variants, .NET), conditionally including the second parameter only when provided. While past comments suggested always including a null placeholder, the current consistent approach appears to be the accepted design.tests/languages/swift/Tests.swift (1)
242-246: Verify test parameter values match expected responses.The past review comments indicate several parameter mismatches with expected OPERATOR_HELPER_RESPONSES. After fixing the method names, verify these parameter values align with the test expectations defined in tests/Base.php.
templates/dotnet/Package/Operator.cs.twig (1)
1-209: LGTM - Solid C# implementation.The .NET Operator template follows C# best practices with proper enum attributes, extension methods for type-safe string conversion, and consistent value normalization. The use of
Client.SerializerOptionsensures serialization consistency across the SDK.templates/web/src/operator.ts.twig (1)
1-269: LGTM - Consistent TypeScript implementation.The Web Operator template maintains consistency with the Deno and Node implementations, providing a unified TypeScript API surface across browser, Deno, and Node.js environments.
templates/android/library/src/main/java/io/package/Operator.kt.twig
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/languages/dart/tests.dart (1)
221-221: Fix remaining mismatched test invocations.Lines 221, 223, and 226 still contain incorrect parameters that don't match the expected OPERATOR_HELPER_RESPONSES referenced in the previous review:
- Line 221:
Operator.multiply(2)should beOperator.multiply(3, 1000)to match expected[3,1000]- Line 223:
Operator.divide(2)should beOperator.divide(4, 1)to match expected[4,1]- Line 226:
Operator.power(2)should beOperator.power(3, 100)to match expected[3,100]Apply this diff to fix the remaining issues:
- print(Operator.multiply(2)); print(Operator.multiply(3, 1000)); - print(Operator.divide(2)); print(Operator.divide(4, 1)); print(Operator.modulo(5)); - print(Operator.power(2)); print(Operator.power(3, 100));Also applies to: 223-223, 226-226
templates/swift/Sources/Operator.swift.twig (2)
13-61: Good: null handling added for decode/encode.
case null,decodeNil(), andencodeNil()are in place.
231-237: Emit explicit null placeholder for arrayFilter value.For cross-SDK parity, when value is omitted, second element should be JSON null (e.g.,
["equals", null]). Current code drops it.Apply:
- public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { - var values: [Any] = [condition.rawValue] - if let value = value { - values.append(value) - } - return Operator(method: "arrayFilter", values: values).description - } + public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { + let placeholder: Any = value ?? NSNull() + return Operator(method: "arrayFilter", values: [condition.rawValue, placeholder]).description + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
templates/deno/mod.ts.twig(2 hunks)templates/go/operator.go.twig(1 hunks)templates/node/src/index.ts.twig(1 hunks)templates/react-native/src/index.ts.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/index.ts.twig(1 hunks)tests/languages/android/Tests.kt(2 hunks)tests/languages/apple/Tests.swift(1 hunks)tests/languages/dart/tests.dart(1 hunks)tests/languages/deno/tests.ts(2 hunks)tests/languages/dotnet/Tests.cs(1 hunks)tests/languages/flutter/tests.dart(1 hunks)tests/languages/go/tests.go(3 hunks)tests/languages/kotlin/Tests.kt(2 hunks)tests/languages/node/test.js(2 hunks)tests/languages/ruby/tests.rb(1 hunks)tests/languages/swift/Tests.swift(1 hunks)tests/languages/web/node.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/languages/go/tests.go
- tests/languages/ruby/tests.rb
- tests/languages/flutter/tests.dart
- tests/languages/web/node.js
🧰 Additional context used
🧬 Code graph analysis (2)
tests/languages/kotlin/Tests.kt (1)
tests/languages/android/Tests.kt (1)
writeToFile(304-307)
tests/languages/android/Tests.kt (1)
tests/languages/kotlin/Tests.kt (1)
writeToFile(271-274)
🔇 Additional comments (16)
tests/languages/apple/Tests.swift (1)
251-277: No issues found with type availability.The verification confirms that
OperatorandConditiontypes are properly exported as public in the Swift templates. TheConditionenum is defined aspublic enum Condition: String, Codablewith theequalcase matching the test usage, and theOperatorstruct is defined aspublic struct Operator : Codable, CustomStringConvertible. These types will be available from the Appwrite or AppwriteEnums imports used in the test file.tests/languages/dotnet/Tests.cs (1)
228-253: Operator and Condition types verified—no issues found.The Operator class and Condition enum both exist in
templates/dotnet/Package/Operator.cs.twig, and the ArrayFilter method signature correctly accepts a Condition parameter followed by an optional value. The test usage on line 247 (Operator.ArrayFilter(Condition.Equal, "test")) matches the method signature. The test pattern is consistent with other helper tests in this file.tests/languages/kotlin/Tests.kt (2)
8-9: LGTM! Imports correctly added.The new imports for
OperatorandConditionare properly added and used in the test block below.
239-265: All test method calls verified and correct.Examined the Operator template (templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig) and confirmed all 25 Operator method calls in the test block (lines 239-265) match their corresponding signatures exactly. Parameter counts, types, and method names are all correct. The code is ready as-is.
templates/node/src/index.ts.twig (1)
10-10: Export looks correct; confirm Condition is a runtime enum.If Condition is a type alias, switch to
export type { Condition }. Tests useCondition.Equal, so it must be a value (enum/const). Also confirmtemplates/node/src/operator.ts.twigis included in mappings.tests/languages/node/test.js (2)
1-2: OK on added imports.Operator/Condition inclusion from dist index matches the new public surface.
Please confirm the generated Node template exposes
Condition.Equal(casing) at runtime.Also applies to: 7-8
329-355: Code verification passed—no issues found.All Operator method definitions (arrayAppend, arrayPrepend, arrayInsert, arrayRemove, arrayUnique, arrayIntersect, arrayDiff) and the Condition enum with Equal value are present and correctly named in the template. Test assertions match the template implementations.
tests/languages/swift/Tests.swift (1)
238-264: Operator test calls updated correctly.Names and parameters match the new Swift Operator API and prior expectations.
Please confirm outputs match OPERATOR_HELPER_RESPONSES in tests/Base.php for Swift.
templates/deno/mod.ts.twig (2)
6-6: LGTM! Import follows established pattern.The Operator and Condition import is consistent with other helper imports in the module.
22-23: LGTM! Exports complete the public API.The Operator and Condition exports properly expose the new helper functionality.
templates/react-native/src/index.ts.twig (1)
11-11: LGTM! Export follows consistent pattern.The Operator and Condition export aligns with the module's export structure.
templates/web/src/index.ts.twig (1)
17-17: LGTM! Export consistent with module structure.The Operator and Condition export properly extends the public API for the Web SDK.
tests/languages/deno/tests.ts (2)
12-13: LGTM! Alias declarations follow established pattern.The Operator and Condition aliases are correctly positioned with other helper aliases.
244-270: No issues found—test method names and Condition enum values are correct.Verification confirms all test calls at lines 244-270 match the generated Operator API:
- All eight array methods (arrayAppend, arrayPrepend, arrayInsert, arrayRemove, arrayUnique, arrayIntersect, arrayDiff, arrayFilter) are defined in the template with matching signatures
- Condition.Equal enum value exists and is properly used in the arrayFilter call at line 263
tests/languages/android/Tests.kt (2)
11-12: LGTM! Imports added correctly.The new imports for
OperatorandConditionare properly structured and consistent with the existing import pattern.
272-298: Test pattern is correct and consistent with codebase conventions.The Operator tests follow the established pattern in the file for helper methods. Operator is a builder class from the
io.appwritedependency that returns serializable objects (similar to the Query helper tests), not service calls that throw exceptions. The test approach of writing output to file for verification, without try-catch blocks, is consistent with other helper method tests in the same file and is appropriate for Kotlin's unchecked exception model.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
templates/dotnet/Package/Operator.cs.twig (1)
12-25: Consider removing redundantEnumMemberattributes.The
Conditionenum hasEnumMemberattributes (lines 12-25), but theToValueextension method uses a manual switch expression to map enum values to strings (lines 32-42). Since the switch statement explicitly handles the conversion, theEnumMemberattributes are redundant and not used anywhere.You can either:
- Remove the
EnumMemberattributes and keep the manual switch (simpler, current approach works fine), or- Remove the manual switch and rely on a serialization-based approach using the
EnumMemberattributesAlso applies to: 30-43
templates/go/operator.go.twig (3)
25-35: Default to empty array when Values is absent (avoid"values": null).Guard against accidental
nilby initializingdata.Valuesto[]ifoptions.Valuesisnil. This keeps the JSON shape stable even if a future call site passes no values.func parseOperator(options operatorOptions) string { data := struct { Method string `json:"method"` Values []interface{} `json:"values"` }{ Method: options.Method, } - if options.Values != nil { - data.Values = *options.Values - } + if options.Values != nil { + data.Values = *options.Values + } else { + data.Values = []interface{}{} + } jsonData, err := json.Marshal(data)
108-114: Normalize nil input slices to[]to prevent"values": null.Public APIs accepting
[]interface{}can receivenil; with the current flow that serializes asnull. Normalize to empty slice for consistent wire format.func ArrayAppend(values []interface{}) string { + if values == nil { + values = []interface{}{} + } return parseOperator(operatorOptions{ Method: "arrayAppend", Values: &values, }) } func ArrayPrepend(values []interface{}) string { + if values == nil { + values = []interface{}{} + } return parseOperator(operatorOptions{ Method: "arrayPrepend", Values: &values, }) } func ArrayIntersect(values []interface{}) string { + if values == nil { + values = []interface{}{} + } return parseOperator(operatorOptions{ Method: "arrayIntersect", Values: &values, }) } func ArrayDiff(values []interface{}) string { + if values == nil { + values = []interface{}{} + } return parseOperator(operatorOptions{ Method: "arrayDiff", Values: &values, }) }Also applies to: 115-121, 146-152, 153-159
8-18: Add brief GoDoc comments for exported API.Consider 1‑liners for
Conditionand each exported function to satisfy GoDoc/lint and improve SDK discoverability.Also applies to: 45-217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)tests/Base.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Base.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: cli (console)
- GitHub Check: dotnet (server)
- GitHub Check: apple (client)
- GitHub Check: react-native (client)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, FlutterBeta)
🔇 Additional comments (3)
templates/dotnet/Package/Operator.cs.twig (1)
76-79: Dependency verified — no issues found.The
Client.SerializerOptionsreference in theToString()method is valid. TheClientclass (defined inClient.cs.twig) contains a public static propertySerializerOptionsof typeJsonSerializerOptionsthat is properly initialized with appropriate serialization settings including custom converters for JSON serialization.templates/go/operator.go.twig (2)
28-28: Good: consistent JSON shape and fail‑fast marshaling.Dropping
omitemptyonvaluesand panicking on marshal failure resolves prior parity and silent‑error concerns. Looks solid.Also applies to: 37-41
160-169: ArrayFilter parity: always emit two elements.Initializing with
[condition, nil]and overwriting when provided matches the expected["<condition>", null]shape. Good fix.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
templates/swift/Sources/Operator.swift.twig (1)
236-242: arrayFilter still omits null placeholder when value is not provided.The past review flagged that
arrayFiltershould emit an explicit null as the second element whenvalueis omitted to maintain cross-SDK consistency. The current implementation only includes the condition when value is nil, resulting in a single-element values array instead of a two-element array with a null placeholder.Based on past review feedback, consider this change:
public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { var values: [Any] = [condition.rawValue] - if let value = value { - values.append(value) - } + values.append(value ?? NSNull()) return Operator(method: "arrayFilter", values: values).description }This ensures the serialized JSON always contains two elements in the values array, matching the expected format across other SDK implementations.
🧹 Nitpick comments (2)
templates/swift/Sources/Operator.swift.twig (1)
156-162: Consider logging encoding failures.The
descriptionproperty silently returns an empty string when JSON encoding fails, which may hide errors during development or debugging.You could optionally add logging or return a fallback that indicates an error occurred:
public var description: String { guard let data = try? JSONEncoder().encode(self) else { + print("Warning: Failed to encode Operator to JSON") return "" } return String(data: data, encoding: .utf8) ?? "" }tests/languages/apple/Tests.swift (1)
251-277: Consider testing arrayFilter without the value parameter.The tests cover all Operator methods, but line 270 only tests
arrayFilterwith a value provided. Given the discussion around null placeholder handling when the value is omitted, adding a test case likeprint(Operator.arrayFilter(Condition.equal))would help verify the output format in that scenario.Optionally add after line 270:
print(Operator.arrayFilter(Condition.equal))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
templates/php/src/Operator.php.twig(1 hunks)templates/ruby/lib/container.rb.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)tests/languages/apple/Tests.swift(1 hunks)tests/languages/dotnet/Tests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/ruby/lib/container/operator.rb.twig
- tests/languages/dotnet/Tests.cs
- templates/php/src/Operator.php.twig
🔇 Additional comments (9)
templates/swift/Sources/Operator.swift.twig (8)
3-11: LGTM!The
Conditionenum is well-structured with clear, descriptive cases for comparison operations.
13-20: LGTM!The
OperatorValueenum now includes the.nullcase, addressing the past review concern about representing JSON nulls.
21-42: LGTM!Decoding logic correctly handles null values (lines 24-25) and provides appropriate fallback error handling.
44-61: LGTM!Encoding logic is complete with all cases covered, including
.nullencoding to JSON null (lines 57-58).
63-140: LGTM!The
Operatorstruct andconvertToOperatorValueArraymethod correctly handle all input types, including preserving empty arrays (lines 111-113) and mapping unknown types to.null(lines 130-133), addressing past review concerns.
164-206: LGTM!The arithmetic operators follow a consistent pattern with appropriate optional
max/minparameters.
208-234: LGTM!Array operators follow a consistent pattern and handle their parameters appropriately.
244-266: LGTM!String, boolean, and date operators are well-implemented with clear, descriptive names and appropriate parameters.
templates/ruby/lib/container.rb.twig (1)
14-14: LGTM. Operator module properly integrated and tested.The operator template exists at
templates/ruby/lib/container/operator.rb.twigand is comprehensively tested intests/languages/ruby/tests.rbwith coverage for increment, decrement, arithmetic operations, array operations, string manipulation, and date operations. The require statement follows the correct pattern and integrates the module appropriately into the SDK initialization.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
templates/dotnet/Package/Operator.cs.twig (2)
115-123: Add division by zero validation.The
Dividemethod does not validate thatdivisoris non-zero. If zero is passed, the operation will fail at runtime when the server or client evaluates this operator.Apply this diff to add validation:
public static string Divide(double divisor, double? min = null) { + if (divisor == 0) + { + throw new ArgumentException("Divisor cannot be zero", nameof(divisor)); + } var values = new List<object> { divisor }; if (min.HasValue) { values.Add(min.Value); } return new Operator("divide", values).ToString(); }
125-128: Add modulo by zero validation.The
Modulomethod does not validate thatdivisoris non-zero, which will cause a runtime exception when evaluated.Apply this diff to add validation:
public static string Modulo(double divisor) { + if (divisor == 0) + { + throw new ArgumentException("Divisor cannot be zero", nameof(divisor)); + } return new Operator("modulo", new List<object> { divisor }).ToString(); }templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
60-92: Critical: Method naming mismatch with test expectations persists.The array operator methods still use the "array" prefix (
arrayAppend,arrayPrepend,arrayInsert,arrayRemove,arrayUnique,arrayIntersect,arrayDiff,arrayFilter), but the test filetests/languages/kotlin/Tests.ktcalls these methods without the prefix (Operator.append,Operator.prepend, etc.). This mismatch will cause compilation failures.Both the method names and the operator name strings (first argument to
Operator(...)) need to be updated to remove the prefix for consistency with test expectations and other language implementations.Apply this diff to fix the naming mismatch:
- fun arrayAppend(values: List<Any>): String { - return Operator("arrayAppend", values).toJson() + fun append(values: List<Any>): String { + return Operator("append", values).toJson() } - fun arrayPrepend(values: List<Any>): String { - return Operator("arrayPrepend", values).toJson() + fun prepend(values: List<Any>): String { + return Operator("prepend", values).toJson() } - fun arrayInsert(index: Int, value: Any): String { - return Operator("arrayInsert", listOf(index, value)).toJson() + fun insert(index: Int, value: Any): String { + return Operator("insert", listOf(index, value)).toJson() } - fun arrayRemove(value: Any): String { - return Operator("arrayRemove", listOf(value)).toJson() + fun remove(value: Any): String { + return Operator("remove", listOf(value)).toJson() } - fun arrayUnique(): String { - return Operator("arrayUnique", emptyList()).toJson() + fun unique(): String { + return Operator("unique", emptyList()).toJson() } - fun arrayIntersect(values: List<Any>): String { - return Operator("arrayIntersect", values).toJson() + fun intersect(values: List<Any>): String { + return Operator("intersect", values).toJson() } - fun arrayDiff(values: List<Any>): String { - return Operator("arrayDiff", values).toJson() + fun diff(values: List<Any>): String { + return Operator("diff", values).toJson() } - fun arrayFilter(condition: Condition, value: Any? = null): String { + fun filter(condition: Condition, value: Any? = null): String { val values = mutableListOf<Any>(condition.value) value?.let { values.add(it) } - return Operator("arrayFilter", values).toJson() + return Operator("filter", values).toJson() }templates/swift/Sources/Operator.swift.twig (1)
238-244: arrayFilter should always include two elements to match other SDKs.The current implementation conditionally appends the value, resulting in a single-element values array when value is nil. According to the past review comment, this should emit an explicit null as the second element to maintain consistency across SDKs.
Based on learnings
Apply this diff to ensure arrayFilter always has two elements:
public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { - var values: [Any] = [condition.rawValue] - if let value = value { - values.append(value) - } - return Operator(method: "arrayFilter", values: values).description + let placeholder: Any = value ?? NSNull() + return Operator(method: "arrayFilter", values: [condition.rawValue, placeholder]).description }
🧹 Nitpick comments (9)
templates/swift/Sources/Operator.swift.twig (1)
158-164: Consider logging encoding failures in description.The
descriptionproperty silently returns an empty string when JSON encoding fails. While this prevents crashes, it might hide underlying issues during development or debugging. Consider whether logging a warning would be beneficial for troubleshooting.templates/dart/lib/operator.dart.twig (1)
34-34: Minor style: Add space afterifkeyword.Dart style guidelines recommend a space between the
ifkeyword and opening parenthesis.Apply this diff:
- if(values != null) { + if (values != null) {templates/deno/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for consistency.While
OperatorValuesListandOperatorValuesare exported,OperatorValuesSingleis not. For API consistency and potential external usage, consider exporting all type aliases.Apply this diff:
-type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesSingle = string | number | boolean;templates/node/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for consistency.Same observation as the Deno version - consider exporting
OperatorValuesSingleto match the export pattern of the other type aliases.Apply this diff:
-type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesSingle = string | number | boolean;templates/react-native/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for consistency.Same pattern across all TypeScript implementations -
OperatorValuesSinglecould be exported for consistency.Apply this diff:
-type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesSingle = string | number | boolean;templates/web/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for consistency.Final TypeScript file with the same export consistency opportunity.
Apply this diff:
-type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesSingle = string | number | boolean;templates/php/src/Operator.php.twig (2)
34-37: Consider error handling for json_encode.The
json_encode()call on line 36 can returnfalseon failure. While unlikely with these simple structures, consider whether error handling or a fallback is needed for robustness.If error handling is desired, consider:
public function __toString(): string { - return json_encode($this); + $json = json_encode($this); + if ($json === false) { + throw new \RuntimeException('Failed to encode Operator: ' . json_last_error_msg()); + } + return $json; }
223-230: Consider validating the condition parameter.The
arrayFilter()method accepts astring $conditionbut doesn't validate that it's one of theConditionclass constants. Depending on your validation strategy, you might want to add a check.Example validation:
public static function arrayFilter(string $condition, mixed $value = null): string { + $validConditions = [ + Condition::Equal, Condition::NotEqual, Condition::GreaterThan, + Condition::GreaterThanEqual, Condition::LessThan, Condition::LessThanEqual, + Condition::Contains, Condition::IsNull, Condition::IsNotNull + ]; + if (!in_array($condition, $validConditions, true)) { + throw new \InvalidArgumentException('Invalid condition: ' . $condition); + } $values = [$condition]; if ($value !== null) { $values[] = $value; } return (new Operator('arrayFilter', $values))->__toString(); }templates/ruby/lib/container/operator.rb.twig (1)
39-39: Prefer idiomaticclass << selffor singleton methods.Using
class << Operatorworks but is unconventional. The Ruby community standard isclass << selfwhen defining singleton methods inside a class.Apply this diff:
- class << Operator + class << self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/python/package/operator.py.twig
- templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: swift (server)
- GitHub Check: kotlin (server)
- GitHub Check: apple (client)
- GitHub Check: android (client)
🔇 Additional comments (23)
templates/swift/Sources/Operator.swift.twig (6)
3-13: LGTM!The Condition enum is well-structured with appropriate String raw values and Codable conformance. The inclusion of
isNullandisNotNullcases extends the condition set appropriately.
15-63: LGTM!The OperatorValue enum implementation is comprehensive and correctly addresses all past review comments:
- Includes the
.nullcase for representing JSON nulls- Properly decodes explicit nulls via
container.decodeNil()- Correctly encodes
.nullto JSON null viacontainer.encodeNil()- Supports nested arrays through recursive
[OperatorValue]case
81-142: LGTM!The
convertToOperatorValueArraymethod properly addresses the past review comment about preserving empty arrays and mapping unknowns to.null:
- Empty arrays are preserved rather than being dropped (lines 113-115)
- Uses
mapinstead ofcompactMapto retain all items (line 118)- NSNull and unknown types are converted to
.null(lines 119-120, 132-134)- Nested empty arrays are preserved via the
?? []fallback (line 130)
166-208: LGTM!The arithmetic operator factory methods follow a consistent and correct pattern:
- Required parameters are always included
- Optional bounds (max/min) are conditionally appended only when provided
- Use of
Doublefor numeric values is appropriate for arithmetic operations
210-237: LGTM!The array manipulation factory methods are well-implemented:
arrayInsertappropriately usesIntfor the index parameterarrayUniquecorrectly uses an empty values array- All methods follow consistent patterns
246-268: LGTM!The string and date operator factory methods are correctly implemented:
replaceproperly accepts both search and replace parameterstoggleanddateSetNowcorrectly use empty values arrays- Date methods appropriately use
Intfor day countstemplates/dart/lib/operator.dart.twig (1)
1-153: Implementation looks solid.The Dart operator builder correctly implements the Condition enum and Operator class with proper JSON serialization. The private constructor pattern and factory methods provide a clean API surface. All operations correctly handle optional parameters and serialize to the expected JSON format.
templates/deno/src/operator.ts.twig (1)
1-271: LGTM - Clean TypeScript implementation.The Deno operator builder is well-structured with proper type definitions, enum, and comprehensive factory methods. JSON serialization logic correctly handles both single values and arrays. All operator methods properly construct their values arrays and delegate to the Operator constructor.
templates/node/src/operator.ts.twig (1)
1-271: LGTM - Implementation matches Deno version.The Node operator builder correctly implements the same pattern as the Deno version, providing consistent operator JSON generation across platforms.
templates/react-native/src/operator.ts.twig (1)
1-271: LGTM - Consistent with other TypeScript implementations.The React Native operator builder maintains the same clean structure and correctness as the Node and Deno versions.
templates/web/src/operator.ts.twig (1)
1-271: LGTM - Web implementation mirrors other TypeScript versions.The Web operator builder is correctly implemented with the same comprehensive operator methods and proper JSON serialization.
templates/go/operator.go.twig (1)
1-219: LGTM - All previous review concerns addressed.The Go operator implementation correctly handles JSON serialization with proper error handling (panic on marshal failure). All operator functions properly construct their values slices and delegate to
parseOperator. The implementation is consistent with the operator patterns in other languages.Previous review comments have been successfully addressed:
- No unused
toArrayfunction- Marshal errors now handled with panic
omitemptyremoved from values JSON tag (line 30)ArrayFilteralways emits 2-element array with nil default (lines 163-166)templates/php/src/Operator.php.twig (5)
5-16: LGTM! Clean constant definitions.The Condition class provides a clear set of comparison operations with consistent naming.
18-32: LGTM! Well-structured constructor with sensible value normalization.The constructor appropriately handles both array and scalar inputs by normalizing them into the internal array representation.
232-263: LGTM! String and boolean operations are well-structured.The
concat(),replace(), andtoggle()methods follow the established pattern correctly.
265-295: LGTM! Date operations follow the established pattern.The date manipulation methods are implemented consistently with the rest of the class.
103-110: ****The
divide()method is a JSON serializer in an SDK code generator, not a mathematical operation. It only builds operation payloads like{"method": "divide", "values": [2]}for server processing. Validation should occur server-side where the actual division executes, not in the client SDK. This design is consistent with all other arithmetic operations in the class (increment, decrement, multiply, power), which similarly lack parameter validation by design.Likely an incorrect or invalid review comment.
templates/ruby/lib/container/operator.rb.twig (6)
1-14: LGTM!The JSON import and Condition module constants are well-structured and follow Ruby naming conventions.
16-37: LGTM!The Operator class initialization and serialization logic correctly handles optional values and conditionally includes the values key in JSON output.
102-106: Great fix from previous review!The
array_filtermethod now correctly omits the optionalvalueparameter when not provided, aligning with the pattern used in other methods.
40-89: LGTM!The arithmetic and array operation methods correctly implement conditional parameter inclusion, ensuring optional parameters are only added to the values array when provided.
108-130: LGTM!The string and date manipulation methods are correctly implemented with appropriate parameter handling.
90-92: The review comment's premise about array_filter is incorrect and should be reconsidered.Based on code inspection across all language templates:
array_filter does NOT pass nil - The review claims it was "fixed to omit optional parameters entirely (passing nil instead)," but the actual implementation shows it builds and passes a values array (either
[condition]or[condition, value]), never nil.All parameterless operations consistently pass empty arrays -
array_unique(),toggle(), anddate_set_now()all pass[]toOperator.new()across Ruby, Python, and TypeScript identically.Operator serialization is consistent - The
Operatorclass only omits thevalueskey from JSON when values is nil/undefined. Passing[]results in"values":[]in the output. Without API documentation specifying the expected format, both approaches appear valid.Given that all implementations are consistent across 13+ language templates and the review's factual claim about array_filter is unsupported, further action depends on whether you have API specifications defining the expected JSON output format for these operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
templates/react-native/src/operator.ts.twig (3)
1-15: Well-defined types and enum.The Condition enum and type definitions are correctly implemented. However,
OperatorValuesSingleis not exported whileOperatorValuesListandOperatorValuesare, creating an inconsistent API surface.Consider exporting
OperatorValuesSinglefor API consistency:-type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesSingle = string | number | boolean;
20-55: Constructor and serialization logic are correct.The value normalization in the constructor properly handles both array and non-array inputs, and the JSON serialization is straightforward and accurate.
Consider making the
valuesproperty private since it's only used internally:export class Operator { method: string; - values: OperatorValuesList | undefined; + private values: OperatorValuesList | undefined;
64-139: Arithmetic operators follow a consistent pattern.All arithmetic factory methods are well-implemented and consistent. However,
divideandmodulooperations don't validate that the divisor is non-zero, which could lead to runtime errors if not handled by the backend.Add client-side validation for division operations:
static divide = (divisor: number, min?: number): string => { + if (divisor === 0) { + throw new Error("Divisor cannot be zero"); + } const values: any[] = [divisor]; if (min !== undefined) { values.push(min); } return new Operator("divide", values).toString(); };static modulo = (divisor: number): string => { + if (divisor === 0) { + throw new Error("Divisor cannot be zero"); + } return new Operator("modulo", [divisor]).toString(); + };templates/php/src/Operator.php.twig (2)
171-174: Consider validating array index bounds.The
arrayInsert()method accepts any integer index without validation. If the backend doesn't handle negative or out-of-bounds indices gracefully, consider adding validation here.If validation is needed, you could add a check like:
public static function arrayInsert(int $index, mixed $value): string { + if ($index < 0) { + throw new \InvalidArgumentException('Index cannot be negative'); + } return (new Operator('arrayInsert', [$index, $value]))->__toString(); }
226-233: Consider validating the condition parameter.The
arrayFilter()method accepts any string for$conditionwithout validating it against theConditionclass constants. This could lead to runtime errors on the backend if an invalid condition is passed.If you want stricter validation, you could add a check:
public static function arrayFilter(string $condition, mixed $value = null): string { + $validConditions = [ + Condition::Equal, Condition::NotEqual, Condition::GreaterThan, + Condition::GreaterThanEqual, Condition::LessThan, Condition::LessThanEqual, + Condition::Contains, Condition::IsNull, Condition::IsNotNull + ]; + if (!in_array($condition, $validConditions, true)) { + throw new \InvalidArgumentException("Invalid condition: {$condition}"); + } $values = [$condition]; if ($value !== null) { $values[] = $value; } return (new Operator('arrayFilter', $values))->__toString(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/node/src/operator.ts.twig
- templates/deno/src/operator.ts.twig
- templates/web/src/operator.ts.twig
- templates/dotnet/Package/Operator.cs.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: apple (client)
- GitHub Check: swift (server)
- GitHub Check: android (client)
🔇 Additional comments (5)
templates/react-native/src/operator.ts.twig (1)
147-270: Array, string, boolean, and date operators are correctly implemented.All factory methods consistently follow the established pattern, properly handle parameters, and generate correct JSON descriptors. The
arrayFiltermethod appropriately converts the Condition enum to a string value.templates/php/src/Operator.php.twig (4)
5-16: LGTM! Clean constant definitions.The Condition class provides a clear set of comparison operation constants with consistent naming.
18-46: LGTM! Well-structured JsonSerializable implementation.The Operator class properly implements JsonSerializable with good value normalization in the constructor and clean serialization logic.
118-124: LGTM! Zero divisor validation properly implemented.The
modulo()method now correctly validates for zero divisor with clear error messaging. This addresses the previous review comment.
48-62: LGTM! Consistent factory method pattern.The static factory methods follow a consistent, clear pattern:
- Proper PHPDoc annotations
- Type hints for parameters and return types
- Conditional handling of optional parameters
- Clean delegation to the Operator constructor
Also applies to: 64-78, 80-94, 126-140, 142-151, 153-162, 176-185, 187-195, 197-206, 208-217, 235-244, 246-256, 258-266, 268-277, 279-288, 290-298
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
templates/dotnet/Package/Operator.cs.twig (1)
117-125: Add division by zero validation.The
Dividemethod still lacks validation fordivisor == 0, which was flagged in a previous review. This is inconsistent with theModulomethod (lines 129-132) which correctly validates for zero. Without this check, the method will produce an invalid operator that will fail when evaluated.Apply this diff to add validation:
public static string Divide(double divisor, double? min = null) { + if (divisor == 0) + { + throw new ArgumentException("Divisor cannot be zero", nameof(divisor)); + } var values = new List<object> { divisor }; if (min.HasValue) { values.Add(min.Value); } return new Operator("divide", values).ToString(); }templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
61-93: Method naming vs tests (array-prefixed vs unprefixed) — please confirmPrevious review flagged tests calling append/prepend/etc. while template defines arrayAppend/arrayPrepend/etc. Validate current tests and align names or test calls.
#!/bin/bash # Compare method names in Android/Kotlin templates vs Kotlin tests echo "Template methods:" rg -n 'fun (array)?(append|prepend|insert|remove|unique|intersect|diff|filter)\(' templates/android/ templates/kotlin/ echo echo "Test calls:" rg -n 'Operator\.(array)?(append|prepend|insert|remove|unique|intersect|diff|filter)\(' tests/ 2>/dev/null || truetemplates/swift/Sources/Operator.swift.twig (1)
239-245: arrayFilter should include explicit null placeholderEnsure second element is present when value is nil to match other SDKs.
- public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { - var values: [Any] = [condition.rawValue] - if let value = value { - values.append(value) - } - return Operator(method: "arrayFilter", values: values).description - } + public static func arrayFilter(_ condition: Condition, value: Any? = nil) -> String { + return Operator(method: "arrayFilter", values: [condition.rawValue, value ?? NSNull()]).description + }
🧹 Nitpick comments (9)
templates/ruby/lib/container/operator.rb.twig (5)
16-27: Consider using.nil?for consistency.The initialization logic is correct. However, for Ruby style consistency, consider using
.nil?instead of!= nil(line 20), which aligns with line 31 and is the more idiomatic Ruby pattern.Optional refactor:
- if values != nil + if !values.nil? if values.is_a?(Array)Note: You may also want to apply this pattern to lines 42, 48, 54, 60, 71, and 105 for consistency throughout the file.
35-37: Minor:self.is unnecessary when calling instance methods.The delegation to
to_jsonworks correctly, butself.is not needed in Ruby when calling instance methods on the current object.Optional refactor:
def to_s - return self.to_json + return to_json end
39-39: Consider usingclass << selffor defining class methods.While
class << Operatoris valid Ruby, the more idiomatic pattern isclass << selfwhen defining class methods inside a class definition.Optional refactor:
- class << Operator + class << self
64-67: Redundant zero check in modulo.The condition
divisor == 0 || divisor == 0.0is redundant because in Ruby,0 == 0.0evaluates totrue. A single check suffices.Optional refactor:
def modulo(divisor) - raise ArgumentError, "Divisor cannot be zero" if divisor == 0 || divisor == 0.0 + raise ArgumentError, "Divisor cannot be zero" if divisor == 0 return Operator.new("modulo", [divisor]).to_s end
75-131: LGTM!The array, string, and date operator methods are well-implemented:
- Array operations correctly handle their respective inputs
- String operations (concat, replace, toggle) are straightforward
- Date operations provide the expected functionality
Minor nitpick: Methods with no parameters (
array_unique,toggle,date_set_now) could omit empty parentheses for more idiomatic Ruby style, but this is purely cosmetic.templates/dotnet/Package/Operator.cs.twig (1)
212-215: Consider adding null validation for string parameters.The
Replacemethod acceptssearchandreplacestring parameters without null checks. While the server will handle null values, adding validation would provide better error messages and fail-fast behavior, consistent with the list parameter validation added elsewhere in this class.Apply this diff to add validation:
public static string Replace(string search, string replace) { + if (search == null) + { + throw new ArgumentNullException(nameof(search)); + } + if (replace == null) + { + throw new ArgumentNullException(nameof(replace)); + } return new Operator("replace", new List<object> { search, replace }).ToString(); }templates/react-native/src/operator.ts.twig (1)
1-3: Simplify OperatorValuesList; any[] subsumes specific array unionsRemoves redundancy; keeps API unchanged.
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = any[]; export type OperatorValues = OperatorValuesSingle | OperatorValuesList;templates/node/src/operator.ts.twig (1)
1-3: Collapse OperatorValuesList to any[]Type union is redundant; any[] already covers it.
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = any[]; export type OperatorValues = OperatorValuesSingle | OperatorValuesList;templates/web/src/operator.ts.twig (1)
1-3: Consider stronger typing for array values.The
any[]type inOperatorValuesListreduces type safety. While this provides flexibility, it allows any array type to be passed. Consider whether the current typing is sufficient for your use case or if stricter constraints would catch more errors at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)tests/languages/web/index.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/go/operator.go.twig
- templates/python/package/operator.py.twig
- templates/dart/lib/operator.dart.twig
- templates/deno/src/operator.ts.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: go (server)
- GitHub Check: build (8.3, Node20)
- GitHub Check: react-native (client)
- GitHub Check: node (server)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: apple (client)
- GitHub Check: android (client)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: web (client)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (13)
templates/ruby/lib/container/operator.rb.twig (3)
3-14: LGTM!The module structure and Condition constants are well-defined and follow Ruby naming conventions.
40-73: LGTM!The arithmetic operator methods (increment, decrement, multiply, divide, power) correctly handle optional parameters and build the appropriate value arrays. The defensive check in
modulofor zero division is good practice.
103-107: ✅ Previous issue resolved.The
array_filtermethod now correctly omits thevalueparameter when it's nil, addressing the concern raised in the previous review.templates/dotnet/Package/Operator.cs.twig (4)
10-50: LGTM! Previous critical issue resolved.The
Conditionenum andConditionExtensions.ToValue()method are now complete. All nine enum members, includingIsNullandIsNotNull, are properly defined and handled in the switch expression. The previous critical issue has been successfully addressed.
52-85: LGTM! Clean implementation of Operator class structure.The class properties, constructors, and serialization logic are well-implemented. The constructor correctly handles both
IListcollections and single values, and theToString()method usesClient.SerializerOptionsfor consistent JSON serialization.
146-205: LGTM! Array operator methods properly implement null validation.All array methods that accept
List<object>parameters now correctly validate for null and throwArgumentNullException. TheArrayFiltermethod properly usescondition.ToValue()to serialize the enum. The previous major issues have been successfully addressed.
217-235: LGTM! Date and toggle operators are correctly implemented.The
Toggle,DateAddDays,DateSubDays, andDateSetNowmethods are properly implemented with appropriate parameter types and no unnecessary validation.templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
19-29: Review comment is incorrect—proposed changes won't achieve stated goalThe suggestion to change
List<Any>toList<Any?>and uselistOf(condition.value, value)won't create the claimed "placeholder" behavior. The GSON configuration inJsonExtensions.kt.twigdoes not call.serializeNulls(), meaning GSON strips null values during serialization. ThuslistOf(condition.value, null)serializes identically to the current conditional-add approach—both produce[condition.value]in JSON. The type annotation alone doesn't control serialization behavior.To actually preserve null placeholders in JSON output,
JsonExtensions.kt.twigwould need.serializeNulls()added to the GsonBuilder. Until then, the current pattern is correct and the proposed changes are cosmetic without the claimed "parity" benefit.Likely an incorrect or invalid review comment.
templates/web/src/operator.ts.twig (3)
5-15: LGTM!The
Conditionenum is well-defined with appropriate comparison and null-check operators for filtering operations.
20-55: LGTM!The
Operatorclass structure is clean and the constructor correctly normalizes single values to arrays. ThetoString()method appropriately serializes to JSON format.
145-275: LGTM!The array, string, boolean, and date operation methods are well-implemented with consistent patterns. The
arrayFiltermethod correctly handles theConditionenum, and all methods appropriately construct and serialize operators.tests/languages/web/index.html (2)
24-24: LGTM!The destructuring correctly includes
OperatorandConditionto support the new operator helper tests.
320-346: LGTM!The operator helper tests comprehensively cover all operator methods including arithmetic operations, array manipulations, string operations, boolean toggles, and date operations. The test cases include both simple and complex parameter scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
templates/react-native/src/operator.ts.twig (1)
218-224: arrayFilter should emit explicit null for cross-SDK parity when value omitted.The current implementation conditionally appends the value, resulting in
["condition"]when value is undefined, rather than["condition", null]. This creates inconsistency with other SDKs (e.g., Go) that always emit a two-element array.templates/android/library/src/main/java/io/package/Operator.kt.twig (2)
90-93: arrayFilter should emit explicit null placeholder for cross-SDK parity.The current implementation conditionally appends the value, resulting in
["condition"]when value is null. For consistency with the Go implementation (templates/go/operator.go.twig lines 183-187), consider always emitting["condition", null].
62-94: Method naming mismatch with test expectations.The methods use an "array" prefix (
arrayAppend,arrayPrepend, etc.), but past review comments indicate tests call these methods without the prefix (append,prepend, etc.). This mismatch will cause compilation failures.templates/node/src/operator.ts.twig (1)
218-224: arrayFilter still needs null placeholder for consistency.The previously flagged issue remains unresolved. The
arrayFiltermethod should always emit a two-element array withnullas the second element when no value is provided, for consistency with other SDKs and test expectations.Apply this diff as previously suggested:
static arrayFilter = (condition: Condition, value?: any): string => { - const values: any[] = [condition as string]; - if (value !== undefined) { - values.push(value); - } - return new Operator("arrayFilter", values).toString(); + return new Operator("arrayFilter", [condition as string, value ?? null]).toString(); };
🧹 Nitpick comments (2)
templates/react-native/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for API consistency.
OperatorValuesSingle(line 1) is not exported whileOperatorValuesListandOperatorValues(lines 2-3) are. If consumers need to reference the single-value type directly, consider adding theexportkeyword.templates/deno/src/operator.ts.twig (1)
1-3: Consider exporting OperatorValuesSingle for API consistency.
OperatorValuesSingleis not exported whileOperatorValuesListandOperatorValuesare. Exporting all related type aliases provides a more consistent public API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/dotnet/Package/Operator.cs.twig
- templates/php/src/Operator.php.twig
🔇 Additional comments (31)
templates/python/package/operator.py.twig (2)
18-29: LGTM! Conditional attribute initialization allows clean JSON omission.The
__init__method only setsself.valueswhen values is notNone, and__str__usesself.__dict__for serialization. This elegantly omits the"values"key from the JSON output when no values are provided, which is a valid design choice for this implementation.
103-107: Cross-SDK inconsistency confirmed; verify whether aligned implementations are intended.The script output confirms the review comment's observation: Python, TypeScript, Ruby, and Deno all omit the second element when the optional value is not provided, producing
["condition"]. However, Go uniquely always initializes with two elements, producing["condition", null]when value is omitted.All non-Go SDKs (Python, TypeScript, Ruby, Deno) follow the same pattern—they only append the second element if it's explicitly provided. This consistency across Python, TypeScript, and Ruby suggests this behavior may be intentional, or that Go is the implementation that diverged and should be reconciled with the others.
The discrepancy warrants clarification: confirm whether Python should maintain consistency with TypeScript and Ruby, or whether all implementations should align with Go's always-present-null-as-second-element approach.
templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
19-29: LGTM! Null-safe type changes applied correctly.The
Operatorclass now usesList<Any?>?for the values parameter (line 21), and all factory methods constructmutableListOf<Any?>(e.g., line 27). This enables proper serialization of explicit null values.templates/deno/src/operator.ts.twig (2)
1-55: LGTM! Well-structured Operator implementation.The type definitions, Condition enum, and Operator class with JSON serialization are correctly implemented. The constructor properly normalizes scalar values to arrays, and the toString method provides clean JSON output.
218-224: arrayFilter should emit explicit null for cross-SDK parity when value omitted.Similar to the React Native implementation, this produces
["condition"]when value is undefined, rather than["condition", null]. For consistency across SDKs, consider always emitting a two-element array.templates/ruby/lib/container/operator.rb.twig (3)
17-37: LGTM! Clean implementation with intentional nil handling.The
initializemethod conditionally sets@values(lines 20-26), andto_jsonomits the:valueskey when nil (line 31). This is a valid design choice that produces cleaner JSON output for operations without values.
104-108: array_filter behavior differs from other SDKs but marked as addressed.The current implementation produces
["condition"]whenvalueis nil, rather than["condition", null]. Past review comments indicate this was addressed in commit e6a0c1f, suggesting this behavior is intentional for Ruby, though it differs from the Go implementation.
58-63: LGTM! Proper zero-division validation.The
dividemethod (lines 58-63) correctly validates against both integer and float zero values before proceeding with the operation.templates/swift/Sources/Operator.swift.twig (3)
15-63: LGTM! Comprehensive null handling implemented.The
OperatorValueenum now includes anullcase (line 21), and theCodableimplementation correctly decodes (lines 26-27) and encodes (lines 59-60) explicit null values. This enables proper cross-SDK null serialization.
81-142: LGTM! Robust array conversion with null preservation.The
convertToOperatorValueArraymethod now preserves empty arrays (lines 113-115), mapsNSNullto.null(lines 119-120), and handles nested arrays without dropping elements (lines 129-131). Unknown types correctly map to.null(lines 132-134).
240-245: arrayFilter should emit explicit null placeholder when value is nil.The current implementation produces
["condition"]when value is nil (line 244). For cross-SDK parity (see Go implementation at templates/go/operator.go.twig lines 183-187), consider always emitting["condition", NSNull()]or similar to produce a two-element array.templates/go/operator.go.twig (3)
27-45: LGTM! Proper error handling and JSON serialization.The
parseOperatorfunction now correctly handles marshal errors with panic (lines 39-42), which is appropriate for template-generated code where marshaling failure indicates a programmer error. TheValuesfield usesjson:"values"withoutomitempty(line 30), ensuring empty arrays are always serialized.
183-192: LGTM! ArrayFilter correctly emits null placeholder for cross-SDK parity.The implementation initializes the values slice with
nilas the second element (line 184) and conditionally overwrites it when a value is provided (lines 185-186). This ensures the serialized output is always["<condition>", null]or["<condition>", <value>], maintaining consistency across SDKs.
80-100: LGTM! Comprehensive zero-division validation.The
Dividefunction properly validates against zero for bothintandfloat64types (lines 82-91), preventing runtime panics from division by zero.templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (3)
19-60: LGTM! Null-safe Operator implementation.The
Operatorclass correctly usesList<Any?>?for the values parameter (line 21), and all factory methods constructmutableListOf<Any?>(e.g., line 27). This enables proper serialization of explicit null values while maintaining type safety.
90-93: Verify intended behavior: arrayFilter omits second element when value is null.The current implementation produces
["condition"]when value is null, whereas the Go implementation (templates/go/operator.go.twig lines 183-187) always produces["condition", null]for cross-SDK consistency. Consider aligning the behavior or documenting the intentional divergence.
44-49: LGTM! Proper zero-division validation.The
dividemethod correctly validates that the divisor is not zero (line 45) usingrequire, which throwsIllegalArgumentExceptionif the condition fails. This prevents runtime division-by-zero errors.templates/dart/lib/operator.dart.twig (5)
3-20: Clean enum implementation.The Condition enum is well-structured with appropriate string values and toString override for serialization.
22-43: Solid implementation of the Operator class.The private constructor pattern and JSON serialization logic are well-implemented and follow Dart best practices.
44-98: Excellent validation in arithmetic operations.The divide and modulo methods correctly validate against zero divisors, and all arithmetic operations follow a consistent pattern with appropriate optional parameters.
101-127: Array operations are well-implemented.The array manipulation methods follow a clean and consistent pattern.
137-160: String and date operations look good.The concat, replace, toggle, and date manipulation methods are correctly implemented.
templates/web/src/operator.ts.twig (5)
1-15: Well-defined types and enum.The type definitions and Condition enum are clean and appropriate for TypeScript.
17-55: Solid Operator class implementation.The constructor's value normalization and JSON serialization are correctly implemented.
57-146: Arithmetic operations are correctly implemented.The zero-divisor validation previously flagged for the
dividemethod has been successfully addressed. Bothdivideandmodulonow include appropriate validation.
148-217: Array operations are well-implemented.The array manipulation methods are clean and follow consistent patterns.
226-277: String and date operations are correct.The concat, replace, toggle, and date manipulation methods are well-implemented.
templates/node/src/operator.ts.twig (4)
1-55: Types, enum, and class structure are well-defined.The TypeScript definitions and Operator class implementation are sound.
57-146: Arithmetic operations are correctly implemented.All mathematical operations, including proper zero-divisor validation for
divideandmodulo, are well-implemented.
148-217: Array operations look good.The array manipulation methods are clean and consistent.
226-277: String and date operations are well-implemented.The concat, replace, toggle, and date methods are correct.
# Conflicts: # tests/languages/web/index.html
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (19)
templates/deno/src/operator.ts.twig (3)
1-3: Simplify value typesThe union includes any[], which subsumes the specific array types. Prefer a single unknown[] for clarity.
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = unknown[]; export type OperatorValues = OperatorValuesSingle | OperatorValuesList;
21-22: Make fields immutablePrevent accidental mutation after construction.
- method: string; - values: OperatorValuesList | undefined; + readonly method: string; + readonly values?: OperatorValuesList;
218-221: Tiny cleanup: drop redundant castCondition is a string enum; no need for as string.
- static arrayFilter = (condition: Condition, value?: any): string => { - const values: any[] = [condition as string, value === undefined ? null : value]; + static arrayFilter = (condition: Condition, value?: any): string => { + const values: any[] = [condition, value === undefined ? null : value]; return new Operator("arrayFilter", values).toString(); };templates/react-native/src/operator.ts.twig (3)
1-3: Simplify value typesAs with Deno, prefer unknown[] over a union that already includes any[].
-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = unknown[]; export type OperatorValues = OperatorValuesSingle | OperatorValuesList;
21-22: Make fields immutableUse readonly to prevent accidental mutation.
- method: string; - values: OperatorValuesList | undefined; + readonly method: string; + readonly values?: OperatorValuesList;
219-219: Drop redundant castCondition is already a string enum.
- const values: any[] = [condition as string, value === undefined ? null : value]; + const values: any[] = [condition, value === undefined ? null : value];templates/ruby/lib/container/operator.rb.twig (2)
1-1: Optional: freeze string literalsAdd the magic comment to reduce string allocations.
+ # frozen_string_literal: true require 'json'
58-68: Idiomatic zero checksMinor style tweak:
- raise ArgumentError, "Divisor cannot be zero" if divisor == 0 || divisor == 0.0 + raise ArgumentError, "Divisor cannot be zero" if divisor.to_f.zero?templates/swift/Sources/Operator.swift.twig (2)
191-206: Prefer throwing/precondition over fatalError for zero divisorCrashing the process is harsh for a serialization helper. Consider throwing an error or using precondition to fail in debug only.
- if divisor == 0 { - fatalError("Divisor cannot be zero") - } + precondition(divisor != 0, "Divisor cannot be zero")Apply similarly in modulo().
158-164: Optional: reuse JSONEncoderdescription creates a new JSONEncoder each call. Consider a static encoder to avoid repeated allocations.
private static let encoder = JSONEncoder() // ... guard let data = try? Operator.encoder.encode(self) else { ... }templates/node/src/operator.ts.twig (3)
1-3: Tighten types; avoid redundant union.
any[]already subsumes the other array unions. Prefer a single, safer alias.-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; -export type OperatorValues = OperatorValuesSingle | OperatorValuesList; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = any[]; // or readonly unknown[] +export type OperatorValues = OperatorValuesSingle | OperatorValuesList;
21-43: Make Operator immutable.Prevents accidental mutation after construction and improves predictability.
-export class Operator { - method: string; - values: OperatorValuesList | undefined; +export class Operator { + readonly method: string; + readonly values: ReadonlyArray<unknown> | undefined; @@ - if (values !== undefined) { - if (Array.isArray(values)) { - this.values = values; - } else { - this.values = [values] as OperatorValuesList; - } - } + if (values !== undefined) { + const arr = Array.isArray(values) ? values : [values]; + this.values = Object.freeze([...arr]); + }
218-221: Minor: drop unnecessary cast.
Conditionis a string enum; theas stringis redundant.- static arrayFilter = (condition: Condition, value?: any): string => { - const values: any[] = [condition as string, value === undefined ? null : value]; + static arrayFilter = (condition: Condition, value?: any): string => { + const values: any[] = [condition, value === undefined ? null : value]; return new Operator("arrayFilter", values).toString(); };templates/go/operator.go.twig (1)
88-99: Broaden zero-divisor checks to all numeric kinds.Current switch only covers
intandfloat64. Calls withint32,int64,uint,float32, etc., can bypass validation.-func Divide(divisor interface{}, min ...interface{}) string { - // Check for zero divisor - switch v := divisor.(type) { - case int: - if v == 0 { - panic("divisor cannot be zero") - } - case float64: - if v == 0.0 { - panic("divisor cannot be zero") - } - } +func Divide(divisor interface{}, min ...interface{}) string { + // Check for zero divisor across common numeric kinds + switch v := divisor.(type) { + case int, int8, int16, int32, int64: + if fmt.Sprintf("%d", v) == "0" { + panic("divisor cannot be zero") + } + case uint, uint8, uint16, uint32, uint64, uintptr: + // Unsigned zeros are still zero + if fmt.Sprintf("%d", v) == "0" { + panic("divisor cannot be zero") + } + case float32: + if v == 0 { + panic("divisor cannot be zero") + } + case float64: + if v == 0 { + panic("divisor cannot be zero") + } + } @@ -func Modulo(divisor interface{}) string { - switch v := divisor.(type) { - case int: - if v == 0 { - panic("divisor cannot be zero") - } - case float64: - if v == 0.0 { - panic("divisor cannot be zero") - } - } +func Modulo(divisor interface{}) string { + switch v := divisor.(type) { + case int, int8, int16, int32, int64: + if fmt.Sprintf("%d", v) == "0" { + panic("divisor cannot be zero") + } + case uint, uint8, uint16, uint32, uint64, uintptr: + if fmt.Sprintf("%d", v) == "0" { + panic("divisor cannot be zero") + } + case float32: + if v == 0 { + panic("divisor cannot be zero") + } + case float64: + if v == 0 { + panic("divisor cannot be zero") + } + }Also applies to: 110-120
templates/web/src/operator.ts.twig (2)
1-3: Simplify list typing.
any[]covers the rest; prefer a single alias (orreadonly unknown[]) to reduce surface area.-type OperatorValuesSingle = string | number | boolean; -export type OperatorValuesList = string[] | number[] | boolean[] | any[]; -export type OperatorValues = OperatorValuesSingle | OperatorValuesList; +type OperatorValuesSingle = string | number | boolean; +export type OperatorValuesList = any[]; +export type OperatorValues = OperatorValuesSingle | OperatorValuesList;
21-43: Make Operator immutable.Same rationale as Node: prevent post-construction mutations.
export class Operator { - method: string; - values: OperatorValuesList | undefined; + readonly method: string; + readonly values: ReadonlyArray<unknown> | undefined; @@ - if (values !== undefined) { - if (Array.isArray(values)) { - this.values = values; - } else { - this.values = [values] as OperatorValuesList; - } - } + if (values !== undefined) { + const arr = Array.isArray(values) ? values : [values]; + this.values = Object.freeze([...arr]); + }templates/php/src/Operator.php.twig (2)
1-1: Enable strict types.Improves type safety across the template outputs.
-<?php +<?php +declare(strict_types=1);
34-37: Harden JSON encoding in __toString.Avoids returning non-string on encoding failure and improves readability; keeps behavior.
- public function __toString(): string - { - return json_encode($this); - } + public function __toString(): string + { + $json = json_encode($this, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + return $json === false ? '' : $json; + }templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
44-53: Stronger divisor validation (NaN/Infinity).Block non-finite divisors to avoid emitting invalid operator payloads.
- fun divide(divisor: Number, min: Number? = null): String { - require(divisor.toDouble() != 0.0) { "Divisor cannot be zero" } + fun divide(divisor: Number, min: Number? = null): String { + val d = divisor.toDouble() + require(d.isFinite() && d != 0.0) { "Divisor must be a finite, non-zero number" } val values = mutableListOf<Any?>(divisor) min?.let { values.add(it) } return Operator("divide", values).toJson() } @@ - fun modulo(divisor: Number): String { - require(divisor.toDouble() != 0.0) { "Divisor cannot be zero" } + fun modulo(divisor: Number): String { + val d = divisor.toDouble() + require(d.isFinite() && d != 0.0) { "Divisor must be a finite, non-zero number" } return Operator("modulo", listOf(divisor)).toJson() }Also applies to: 51-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/SDK/Language/Apple.php(1 hunks)src/SDK/Language/Web.php(1 hunks)templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/index.ts.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)tests/languages/web/index.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/SDK/Language/Apple.php
- templates/python/package/operator.py.twig
- templates/web/src/index.ts.twig
- src/SDK/Language/Web.php
- templates/dotnet/Package/Operator.cs.twig
- templates/dart/lib/operator.dart.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: kotlin (server)
- GitHub Check: android (client)
- GitHub Check: swift (server)
- GitHub Check: apple (client)
- GitHub Check: flutter (client)
🔇 Additional comments (8)
tests/languages/web/index.html (2)
24-24: Import surface LGTMIncluding Operator and Condition in the Appwrite import is correct for the new helpers.
341-341: arrayFilter call matches new APIUsing Condition.Equal and passing the value arg keeps cross‑SDK parity with ["equal", "test"].
templates/react-native/src/operator.ts.twig (1)
218-221: arrayFilter parity: goodEmitting ["condition", null] when value is omitted matches other SDKs.
templates/ruby/lib/container/operator.rb.twig (1)
104-107: array_filter null placeholder is correctKeeping [condition, nil] (→ ["condition", null]) ensures parity with other SDKs.
templates/swift/Sources/Operator.swift.twig (1)
15-22: Null handling implemented correctlyOperatorValue includes .null with proper encode/decode; this preserves ["condition", null] and nested nulls.
Also applies to: 26-61
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (1)
62-90: No issues found—Kotlin tests correctly use array-prefixed function names.Verification confirms that the Kotlin test file (
./tests/languages/kotlin/Tests.kt) consistently calls the prefixed array functions:Operator.arrayAppend(),Operator.arrayPrepend(),Operator.arrayInsert(),Operator.arrayRemove(),Operator.arrayUnique(),Operator.arrayIntersect(),Operator.arrayDiff(), andOperator.arrayFilter(). No unprefixed variants were found. Test parity is maintained.templates/android/library/src/main/java/io/package/Operator.kt.twig (2)
62-90: No naming parity issues found—review comment is not substantiated.Android tests (lines 284–291) call the array-prefixed methods (
arrayAppend,arrayPrepend,arrayInsert, etc.), which match the method signatures defined in the Kotlin template (lines 62–90). The concern raised in the review comment is not supported by the current codebase.
90-93: Review comment is based on incorrect assumption about Gson default behavior.The code does not rely on Gson including nulls—it relies on the opposite. By default Gson omits (does not serialize) fields whose value is null. The codebase's
GsonBuilderconfiguration does not call.serializeNulls(), so whenarrayFilterpasses a null value in the list, it will be excluded from the JSON output, not included. The concern raised in the review is not applicable.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
73-99: Method naming may mismatch tests (array vs unprefixed). Please verify*Past tests referred to append/prepend/insert/remove/unique/intersect/diff/filter without the "array" prefix, which would fail to compile against these APIs. Either add aliases or update tests for consistency.
Run:
#!/bin/bash # Check Kotlin/Android tests for Operator calls without 'array' prefix rg -nP 'Operator\.(append|prepend|insert|remove|unique|intersect|diff|filter)\s*\(' tests/ -C2 || true # Check templates for provided method names rg -nP 'fun\s+(arrayAppend|arrayPrepend|arrayInsert|arrayRemove|arrayUnique|arrayIntersect|arrayDiff|arrayFilter)\s*\(' templates/android/ templates/kotlin/ -C2
🧹 Nitpick comments (5)
templates/dart/lib/operator.dart.twig (1)
24-36: Narrowvaluestyping and simplifytoJsonAll call sites pass a List; you can drop the dynamic branch and make serialization clearer.
Apply:
- final dynamic values; + final List<dynamic>? values; - - Operator._(this.method, [this.values = null]); + Operator._(this.method, [this.values]); @@ - if(values != null) { - result['values'] = values is List ? values : [values]; - } + if (values != null) { + result['values'] = values; + }templates/python/package/operator.py.twig (1)
25-31: Prefer explicit dict over serializing__dict__Safer and clearer than relying on
defaultfor objects.Apply:
- def __str__(self): - return json.dumps( - self.__dict__, - separators=(",", ":"), - default=lambda obj: obj.__dict__ - ) + def __str__(self): + data = {"method": self.method} + if hasattr(self, "values"): + data["values"] = self.values + return json.dumps(data, separators=(",", ":"))templates/deno/src/operator.ts.twig (1)
251-254: SimplifyarrayFilterwith nullish coalescing; drop unnecessary castSame behavior, less code.
Apply:
- static arrayFilter = (condition: Condition, value?: any): string => { - const values: any[] = [condition as string, value === undefined ? null : value]; - return new Operator("arrayFilter", values).toString(); - }; + static arrayFilter = (condition: Condition, value?: any): string => + new Operator("arrayFilter", [condition, value ?? null]).toString();templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
81-87: Allow null values for insert/remove to align with List<Any?>Signatures currently forbid nulls while other APIs accept nullable list elements.
Apply:
- fun arrayInsert(index: Int, value: Any): String { + fun arrayInsert(index: Int, value: Any?): String { return Operator("arrayInsert", listOf(index, value)).toJson() } @@ - fun arrayRemove(value: Any): String { + fun arrayRemove(value: Any?): String { return Operator("arrayRemove", listOf(value)).toJson() }Also applies to: 85-87
templates/react-native/src/operator.ts.twig (1)
251-254: TightenarrayFilterimplementationUse nullish coalescing and avoid the cast.
Apply:
- static arrayFilter = (condition: Condition, value?: any): string => { - const values: any[] = [condition as string, value === undefined ? null : value]; - return new Operator("arrayFilter", values).toString(); - }; + static arrayFilter = (condition: Condition, value?: any): string => + new Operator("arrayFilter", [condition, value ?? null]).toString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
templates/android/library/src/main/java/io/package/Operator.kt.twig(1 hunks)templates/dart/lib/operator.dart.twig(1 hunks)templates/deno/src/operator.ts.twig(1 hunks)templates/dotnet/Package/Operator.cs.twig(1 hunks)templates/go/operator.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig(1 hunks)templates/node/src/operator.ts.twig(1 hunks)templates/php/src/Operator.php.twig(1 hunks)templates/python/package/operator.py.twig(1 hunks)templates/react-native/src/operator.ts.twig(1 hunks)templates/ruby/lib/container/operator.rb.twig(1 hunks)templates/swift/Sources/Operator.swift.twig(1 hunks)templates/web/src/operator.ts.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/dotnet/Package/Operator.cs.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: kotlin (server)
- GitHub Check: swift (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (12)
templates/kotlin/src/main/kotlin/io/appwrite/Operator.kt.twig (3)
26-71: Excellent validation for arithmetic operations.The comprehensive NaN, Infinity, and zero-division checks across all numeric operations demonstrate good defensive programming. The consistent validation pattern and use of Kotlin's
require()make the code robust and idiomatic.
73-104: Verify nullable type consistency in array operations.There's an inconsistency in how nullable values are handled:
arrayAppend,arrayPrepend,arrayIntersect,arrayDiffacceptList<Any?>(nullable elements allowed)arrayInsertandarrayRemoverequire non-nullAnyarrayFilteraccepts nullableAny?Is there a reason why you can append/prepend null values but cannot insert or remove them? This might confuse users if the restriction is unintentional.
Consider standardizing to either allow or disallow null values consistently across all single-element array operations:
// Option 1: Allow nulls everywhere fun arrayInsert(index: Int, value: Any?): String { return Operator("arrayInsert", listOf(index, value)).toJson() } fun arrayRemove(value: Any?): String { return Operator("arrayRemove", listOf(value)).toJson() } // Option 2: Explicitly document the restriction if intentional
50-57: Past review concern already addressed.Regarding the previous comment asking about NaN/Infinity validation on line 53: the validation is already present on lines 51-52, which check both the divisor and optional min parameter before the zero-division check on line 53. The implementation correctly validates all edge cases.
templates/dart/lib/operator.dart.twig (1)
161-165: arrayFilter parity looks correctEmits the two-slot array with a null placeholder when value is omitted. Matches cross-SDK behavior.
templates/python/package/operator.py.twig (1)
125-129: array_filter parity confirmedAlways emits [condition, value] with None serialized as null when omitted.
templates/android/library/src/main/java/io/package/Operator.kt.twig (1)
101-104: arrayFilter parity looks goodEmits [condition, value] with null preserved when value omitted.
templates/swift/Sources/Operator.swift.twig (1)
1-305: LGTM! Well-structured Swift Operator implementation.The implementation correctly addresses all previously flagged issues:
- Null handling with explicit
.nullcase (line 21) and proper encoding/decoding (lines 26-27, 59-60)- Empty array preservation and unknown-to-null mapping in
convertToOperatorValueArray(lines 113-115, 132-135)- Explicit null placeholder in
arrayFilter(line 278)- Comprehensive validation for arithmetic operations (NaN/Infinity checks, zero-divisor guards)
The API surface is consistent with other language implementations in this PR and provides robust type safety through the
OperatorValueenum.templates/node/src/operator.ts.twig (1)
1-308: LGTM! Clean TypeScript implementation with proper validation.The Node operator template is well-implemented with:
- Clear type definitions for operator values (lines 1-3)
- Comprehensive
Conditionenum including null checks (lines 5-15)- Consistent constructor normalization (lines 36-42)
- Proper validation for all arithmetic operations (NaN, Infinity, zero-divisor checks)
- Correct null placeholder handling in
arrayFilter(line 252)The implementation aligns well with the other language templates in this PR and provides a clean, type-safe API for operator construction.
templates/ruby/lib/container/operator.rb.twig (1)
1-145: LGTM! Solid Ruby implementation with comprehensive validation.The Ruby operator template provides:
- Clean module structure with
Conditionconstants (lines 3-14)- Proper value normalization in the constructor (lines 20-26)
- JSON serialization with conditional value inclusion (lines 29-33)
- Thorough validation for arithmetic operations using
nan?andinfinite?checks- Zero-divisor guards for
divideandmodulo(lines 67, 75)- Consistent
array_filterbehavior that always includes the value parameter (line 116), matching other SDK implementationsThe implementation follows Ruby idioms and maintains parity with the other language templates.
templates/go/operator.go.twig (1)
1-277: LGTM! Clean Go implementation with proper error handling.The Go operator template successfully addresses all previously flagged issues:
- Removed unused helper functions (no
toArraypresent)- Marshal errors properly handled with panic (lines 53-56)
- Removed
omitemptyfrom values field to ensure empty arrays serialize (line 44)ArrayFilteralways emits two-element array with explicit nil placeholder (line 221)Additional strengths:
- Idiomatic Go patterns with exported/unexported naming conventions
- Centralized numeric validation helper (lines 28-39)
- Comprehensive zero-divisor checks in
DivideandModulo(lines 115-125, 138-146)- Clean separation between public API (exported functions) and internal serialization logic
templates/php/src/Operator.php.twig (1)
1-331: LGTM! Comprehensive PHP implementation with proper type safety.The PHP operator template successfully resolves all previously identified issues:
- Zero-divisor validation in both
divide(lines 128-130) andmodulo(lines 149-151)- Consistent NaN/Infinity checks across all arithmetic operations
- Proper null handling in
arrayFilter(line 263)Additional strengths:
- Modern PHP type hints (
int|float,mixed) for better IDE support and runtime safetyJsonSerializableimplementation for clean serialization (lines 39-45)- Comprehensive PHPDoc comments for all public methods
- Value normalization in constructor (lines 27-31) handles both arrays and scalar values
- Consistent factory method pattern returning serialized strings
The implementation provides a clean, type-safe API that aligns well with PHP best practices and the other language templates in this PR.
templates/web/src/operator.ts.twig (1)
1-308: LGTM! Solid Web TypeScript implementation.The Web operator template successfully addresses all previously flagged issues:
- Division by zero validation in
divide(lines 134-136)- Zero-divisor check in
modulo(lines 154-156)- Correct null placeholder handling in
arrayFilter(line 252)The implementation mirrors the Node template structure, which is appropriate for maintaining consistency across JavaScript/TypeScript environments. All arithmetic operations include comprehensive validation (NaN, Infinity, zero-divisor checks), and the type definitions provide good developer experience with IntelliSense support.
The factory method pattern returning JSON strings is clean and consistent with the broader PR's cross-language operator API design.
2 participants
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Tests