display class of record in custom formatter #242
Conversation
WalkthroughThe updates bring about significant changes across configuration and source files, focusing on version increments, performance optimization, and code readability enhancements. These changes cover updating package versions in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockyarn.lockis excluded by!**/*.lock
Files selected for processing (6)
- Cargo.toml (1 hunks)
- package.json (2 hunks)
- src/cirru/calcit-core.cirru (1 hunks)
- ts-src/custom-formatter.mts (2 hunks)
- ts-src/js-cirru.mts (5 hunks)
- ts-src/js-list.mts (4 hunks)
Files skipped from review due to trivial changes (1)
- Cargo.toml
Additional comments not posted (11)
package.json (2)
3-3: The version update is appropriate for the changes described.
26-26: Ensure compatibility with the updated@cirru/writer.tsdependency.ts-src/custom-formatter.mts (2)
109-110: Optimizing loop iteration by pre-storing the length of the object is a good performance improvement.
184-196: The adjustments in conditional branching forCalcitRecordinstances based on the presence ofobj.klassenhance the visibility and differentiation of values.ts-src/js-list.mts (3)
110-111: Using a variable to store the length of the list before iterating through it in thenestedDataInChildrenmethod is a good performance optimization.
297-298: Optimizing thefoldlfunction by pre-storing the length of the list improves performance.
341-342: Pre-storing the length of the list in thefoldl_shortcutfunction is a beneficial performance optimization.ts-src/js-cirru.mts (3)
85-96: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-93]
Refactoring in the
to_cirru_ednfunction, including the use of template literals and optimized array processing, improves performance and readability.
171-181: The introduction of therecordFieldOrderfunction for consistent sorting of record fields enhances readability and maintainability.
335-335: Ensure the change in howbaseClassis initialized in theextract_cirru_ednfunction effectively improves tuple handling.src/cirru/calcit-core.cirru (1)
462-462: Ensure the use ofidentical?instead of&=aligns with the intended behavior for tag matching in tuples. This change shifts from equality to identity checking, which might affect pattern matching outcomes.
There was a problem hiding this comment.
Actionable comments posted: 12
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (8)
- Cargo.toml (1 hunks)
- package.json (2 hunks)
- ts-src/calcit-data.mts (4 hunks)
- ts-src/calcit.procs.mts (3 hunks)
- ts-src/custom-formatter.mts (4 hunks)
- ts-src/js-cirru.mts (5 hunks)
- ts-src/js-map.mts (4 hunks)
- ts-src/js-record.mts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- package.json
- ts-src/custom-formatter.mts
- ts-src/js-cirru.mts
ts-src/calcit.procs.mts
Outdated
| for (let k in a.listeners) { | ||
| let f = a.listeners.get(k); |
There was a problem hiding this comment.
Consider using a.listeners.forEach for iterating over listeners for improved readability and performance.
- for (let k in a.listeners) {
- let f = a.listeners.get(k);
+ a.listeners.forEach((f, k) => {
f(v, prev);
- }
+ });This change leverages the Map.forEach method, which is more idiomatic for iterating over entries in a Map object. It directly provides access to both the key and value, eliminating the need for a separate get call.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| for (let k in a.listeners) { | |
| let f = a.listeners.get(k); | |
| a.listeners.forEach((f, k) => { | |
| f(v, prev); | |
| }); |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
When destructuring arrays, consider using more descriptive variable names for readability.
- let k = pairs[idx][0];
- let v = pairs[idx][1];
+ let [key, value] = pairs[idx];This change makes the code more readable by clearly indicating the purpose of the variables. It also simplifies the assignment by using array destructuring.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; | |
| let [key, value] = pairs[idx]; |
| let klass = pair[0]; | ||
| let tag = pair[1]; |
There was a problem hiding this comment.
Ensure that the destructuring of the pair into klass and tag is correctly handling cases where lookup_class might return null.
- let pair = lookup_class(obj);
- if (pair == null) {
+ let [klass, tag] = lookup_class(obj) || [];
+ if (!klass || !tag) {
throw new Error(`No class for ${obj?.toString() || JSON.stringify(obj)} to lookup .${p}`);
}This change adds a fallback to an empty array when lookup_class returns null, preventing potential runtime errors when attempting to destructure null or undefined. It also checks for the presence of both klass and tag before proceeding, ensuring that the method does not proceed with undefined values.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let klass = pair[0]; | |
| let tag = pair[1]; | |
| let [klass, tag] = lookup_class(obj) || []; | |
| if (!klass || !tag) { |
| let k = pairs[i][0]; | ||
| let v = pairs[i][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) enhances clarity and may improve performance by avoiding the creation of temporary arrays.
- let [k, v] = pairs[i];
+ let k = pairs[i][0];
+ let v = pairs[i][1];This change aligns with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[i][0]; | |
| let v = pairs[i][1]; | |
| let k = pairs[i][0]; | |
| let v = pairs[i][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the toString method of CalcitMap class enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the nestedDataInChildren method of CalcitSliceMap class enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; | |
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the hashFunction for CalcitSliceMap enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the hashFunction for CalcitMap enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the to_js_data function for CalcitMap and CalcitSliceMap enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; | |
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
| let k = pairs[idx][0]; | ||
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Refactoring the destructuring assignment to individual assignments for key (k) and value (v) in the _$n__$e_ comparison function for CalcitMap and CalcitSliceMap enhances clarity and may improve performance.
- let [k, v] = pairs[idx];
+ let k = pairs[idx][0];
+ let v = pairs[idx][1];This change is consistent with the PR's objective to improve readability and performance. Ensure similar changes are consistently applied across the codebase for uniformity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; | |
| let k = pairs[idx][0]; | |
| let v = pairs[idx][1]; |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (5)
- Cargo.toml (1 hunks)
- calcit/test.cirru (2 hunks)
- package.json (2 hunks)
- ts-src/calcit-data.mts (5 hunks)
- ts-src/calcit.procs.mts (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- package.json
- ts-src/calcit-data.mts
- ts-src/calcit.procs.mts
Additional comments not posted (2)
calcit/test.cirru (2)
205-205: Ensure the new parameter order in theadd-watchfunction call is correct and aligns with its expected signature.
222-229: The addition of atoms*band*c, along with the watch functionality and assertions, appears logically sound. Ensure thorough testing is conducted to validate the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/bin/cr.rs (3 hunks)
- src/cli_args.rs (1 hunks)
- src/codegen.rs (2 hunks)
- src/codegen/emit_js.rs (2 hunks)
Additional comments not posted (8)
src/codegen.rs (3)
10-10: LGTM! UsingAtomicBoolfor thread-safe global flags is appropriate.
25-30: Consider adding documentation toset_code_gen_skip_arity_checkto explain its purpose and the implications of skipping arity checks.
33-35: LGTM! Theskip_arity_checkfunction is implemented correctly.src/cli_args.rs (1)
33-38: LGTM! The addition of theskip-arity-checkCLI argument is implemented correctly. Consider enhancing the help text to include guidance on when it might be appropriate to use this flag.src/bin/cr.rs (3)
33-33: LGTM! The addition of theskip_arity_checkfield to theCLIOptionsstruct is clear and follows Rust conventions.
52-52: LGTM! Theskip_arity_checkfield is correctly initialized based on the CLI argument.
161-163: LGTM! The conditional check forskip_arity_checkis implemented correctly. Consider adding a comment explaining the purpose of this check for future maintainers.src/codegen/emit_js.rs (1)
1046-1048: Consider adding a fallback mechanism or configuration option forskip_arity_checkto ensure flexibility in arity checking behavior.While the current implementation allows for skipping arity checks based on the
skip_arity_checkfunction, it might be beneficial to provide a way to configure this behavior more granularly. This could involve passing a parameter togen_js_funcor using a configuration setting that can be adjusted based on the needs of different environments or debugging scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockyarn.lockis excluded by!**/*.lock
Files selected for processing (3)
- Cargo.toml (1 hunks)
- package.json (2 hunks)
- ts-src/calcit-data.mts (7 hunks)
Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- package.json
- ts-src/calcit-data.mts
make difference of value with/without class more obvious.
Summary by CodeRabbit
@calcit/procsand@cirru/writer.tsto newer versions.