👍 add eval module and mark deprecated helper/expr_string
#260
Conversation
WalkthroughThis update introduces new modules and enhancements for managing Vim expressions and strings in TypeScript, including new utility functions and types. It also features updated dependency versions and deprecation notices for outdated functionalities, promoting cleaner integration with Vim and clearer development practices. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Deno
participant Vim
Developer->>Deno: Invoke expression evaluation
Deno->>Vim: Execute Vim command
Vim-->>Deno: Return result
Deno-->>Developer: Send evaluated result
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 Configuration 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: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- deno.jsonc (3 hunks)
- eval/expression.ts (1 hunks)
- eval/expression_test.ts (1 hunks)
- eval/mod.ts (1 hunks)
- eval/string.ts (1 hunks)
- eval/string_test.ts (1 hunks)
- eval/stringify.ts (1 hunks)
- eval/stringify_test.ts (1 hunks)
- eval/use_eval.ts (1 hunks)
- eval/use_eval_test.ts (1 hunks)
- eval/vim_evaluatable.ts (1 hunks)
- eval/vim_evaluatable_test.ts (1 hunks)
- helper/expr_string.ts (4 hunks)
- helper/keymap.ts (5 hunks)
- helper/keymap_test.ts (1 hunks)
- lambda/mod.ts (6 hunks)
- lambda/mod_test.ts (1 hunks)
Additional context used
Biome
eval/expression.ts
[error] 101-101: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 104-106: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
eval/use_eval.ts
[error] 31-31: Constraining a type parameter to any or unknown is useless.
All types are subtypes of any and unknown.
Safe fix: Remove the constraint.(lint/complexity/noUselessTypeConstraint)
eval/string.ts
[error] 134-134: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 139-141: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
eval/stringify_test.ts
[error] 202-202: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (61)
eval/vim_evaluatable.ts (1)
1-20: LGTM!The code is well-structured and makes effective use of TypeScript's type system. The use of
Symbol.foris appropriate for ensuring consistency across library versions. The predicate function is correctly implemented.eval/vim_evaluatable_test.ts (1)
1-32: LGTM!The tests are comprehensive and well-organized, covering both positive and negative cases for the
isVimEvaluatablefunction. The use oft.stepfor subtests is a good practice.eval/mod.ts (1)
1-38: LGTM!The module-level comment is clear and provides a good overview of how to use the exported functions. The exports are consistent with the PR objectives.
eval/string_test.ts (3)
8-13: LGTM! The test case forrawString()is well-implemented.The test correctly checks the
Symbol.toStringTagto verify the type.
15-44: Comprehensive test case forRawString.The test covers important aspects like
@@vimExpressionOf(),toJSON(),toString(), and inspection.
46-71: Thorough test coverage forisRawString().The test cases effectively verify the function's behavior with different data types.
eval/expression_test.ts (3)
8-13: LGTM! The test case forexpr()is well-implemented.The test correctly checks the
Symbol.toStringTagto verify the type.
15-44: Comprehensive test case forExpression.The test covers important aspects like
@@vimExpressionOf(),toJSON(),toString(), and inspection.
47-72: Thorough test coverage forisExpression().The test cases effectively verify the function's behavior with different data types.
helper/keymap.ts (3)
5-9: LGTM! Import statements are correctly updated.The imports reflect the new module structure and dependencies.
Line range hint
12-23:
LGTM! Changes toKeystype andtoKeysfunction enhance flexibility.The inclusion of
RawStringand the check forisRawStringare well-implemented.
71-83: LGTM! Thesendfunction is well-refactored.The use of
useEvaland the new promise handling approach improve the function's logic.helper/keymap_test.ts (9)
13-18: Test case for normal keys with string looks good.The test case correctly verifies the behavior of sending normal keys using a string.
19-24: Test case for normal keys with string[] looks good.The test case correctly verifies the behavior of sending normal keys using an array of strings.
25-29: Test case for special keys with RawString looks good.The test case correctly verifies the behavior of sending special keys using
RawString.
30-38: Test case for special keys with RawString[] looks good.The test case correctly verifies the behavior of sending special keys using an array of
RawString.
39-43: Test case for special keys with ExprString looks good.The test case correctly verifies the behavior of sending special keys using
ExprString.
44-52: Test case for special keys with ExprString[] looks good.The test case correctly verifies the behavior of sending special keys using an array of
ExprString.
53-61: Test case for non-remapped keys with Keys looks good.The test case correctly verifies the behavior of sending non-remapped keys using
Keys. The use ofAsyncDisposableStackensures proper cleanup.
62-70: Test case for remapped keys with Keys looks good.The test case correctly verifies the behavior of sending remapped keys using
Keys. The use ofAsyncDisposableStackensures proper cleanup.
71-83: Test case for mixed keys with Keys[] looks good.The test case correctly verifies the behavior of sending mixed keys using an array of
Keys. The use ofAsyncDisposableStackensures proper cleanup.eval/expression.ts (3)
26-35: Expression type and ExpressionProps interface look good.The
Expressiontype andExpressionPropsinterface are well-defined and correctly implement thetoJSONmethod andSymbol.toStringTag.
66-73: expr function implementation looks good.The
exprfunction correctly creates anExpressionfrom a template string and substitutions, ensuring safe conversion.
87-93: isExpression predicate looks good.The
isExpressionpredicate is correctly defined to check if a value is anExpression.deno.jsonc (3)
11-11: New export path for./evallooks good.The addition of the export path for
./evalis correct and aligns with the new module introduction.
45-51: Import updates look good.The version update for
@core/unknownutiland the addition of@nick/disposeare appropriate and align with the project's needs.
66-66: New import path forjsr:@denops/std/evallooks good.The addition of the import path for
jsr:@denops/std/evalis correct and aligns with the new module introduction.eval/use_eval.ts (3)
41-56: LGTM!The
ensurePrerequisitesfunction correctly sets up the necessary script in theDenopscontext.
58-61: LGTM!The
trimEndOfArgsfunction efficiently removes trailingundefinedvalues from an array.
63-129: LGTM!The
UseEvalHelperclass is well-structured and correctly extendsDenopsfunctionality with additional methods for evaluation support.eval/string.ts (2)
32-32: LGTM!The
RawStringtype is appropriately defined to represent a Vim string constant with additional properties.
99-105: LGTM!The
rawStringfunction correctly creates aRawStringfrom a template string usingString.raw.eval/stringify.ts (4)
69-137: LGTM!The
stringifyfunction effectively converts JavaScript values to a Vim-evaluable string format, handling various types and circular references.
148-150: LGTM!The
isJsonablefunction correctly identifies objects that can be serialized with atoJSONmethod.
152-154: LGTM!The
isIgnoreRecordValuefunction correctly identifies values that should be ignored in records.
171-182: LGTM!The
toBlobLiteralfunction efficiently converts anArrayBufferto a Vim blob literal.lambda/mod.ts (5)
35-45: Review Import Statements.The import statements have been updated to include new types and functions from the
evalmodule. Ensure these are used appropriately within the file.
157-157: ExtendLambdawithDisposable.The
Lambdainterface now extendsDisposable, which is a good practice for resource management. Ensure that all instances ofLambdaare properly disposed of to prevent resource leaks.
Line range hint
175-192:
Update Return Type toExpression.The
notifyandrequestmethods now return anExpressioninstead of astring. This change enhances type safety and aligns with the newevalmodule's functionality.
258-283: RefactoraddFunction withfnWrapper.The
addfunction now uses afnWrapperto handle different argument types more gracefully. This improves flexibility and usability. The use ofexprfor constructing notifications and requests is a good enhancement.
283-283: Avoid UsingdeleteOperator.The use of the
deleteoperator can impact performance. Consider setting the property toundefinedinstead.- delete denops.dispatcher[id]; + denops.dispatcher[id] = undefined;Likely invalid or redundant comment.
helper/expr_string.ts (4)
26-26: DeprecateExprStringType.The
ExprStringtype is deprecated. Developers should transition to usingrawStringandRawStringfor better compatibility with the newevalmodule.
52-53: DeprecateExprStringUsage.The deprecation notice for
ExprStringencourages the use ofrawStringandRawString. This aligns with the new module's approach.
105-105: DeprecateexprQuoteFunction.The
exprQuotefunction is deprecated in favor ofrawString. This change aims to streamline the handling of Vim's string constants.
130-131: DeprecateisExprStringFunction.The
isExprStringfunction is deprecated. UseisRawStringinstead to check forRawStringtypes.eval/stringify_test.ts (3)
1-8: Comprehensive Test Setup.The test setup includes necessary imports for assertions and mocks, ensuring a robust testing environment.
9-261: Thorough Test Coverage forstringify().The test cases for
stringify()are comprehensive, covering various data types and edge cases. This ensures the function's reliability and correctness.Tools
Biome
[error] 202-202: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
263-300: Vim Evaluation Tests forstringify().These tests ensure that the
stringify()function produces output that can be correctly evaluated in Vim, which is crucial for its intended use.lambda/mod_test.ts (5)
20-126: Comprehensive test coverage forregisterfunction.The test cases for the
registerfunction are well-structured and cover synchronous, asynchronous, and error scenarios effectively.
128-165: Thorough test coverage forunregisterfunction.The test cases for the
unregisterfunction ensure correct behavior for both registered and non-registered IDs.
168-195: Effective test coverage foraddfunction.The test cases for the
addfunction ensure correct registration and disposal of lambda functions.
196-326: Comprehensive test coverage forLambda.notifyandLambda.request.The test cases for
Lambda.notifyandLambda.requestcover various argument types and return values effectively.
329-388: Thorough test coverage forLambda.dispose.The test cases for
Lambda.disposeensure correct behavior for disposing lambda functions.eval/use_eval_test.ts (8)
16-74: Comprehensive test coverage foruseEval.The test cases for
useEvalcover executor calls, value resolution, and error handling effectively.
81-88: Effective test coverage forUseEvalHelper.redraw.The test case ensures correct behavior for calling
denops.redraw.
90-245: Comprehensive test coverage forUseEvalHelper.call.The test cases for
UseEvalHelper.callcover various argument types effectively.
247-391: Comprehensive test coverage forUseEvalHelper.cmd.The test cases for
UseEvalHelper.cmdcover various contexts effectively.
393-551: Comprehensive test coverage forUseEvalHelper.eval.The test cases for
UseEvalHelper.evalcover various contexts effectively.
553-760: Comprehensive test coverage forUseEvalHelper.batch.The test cases for
UseEvalHelper.batchcover various argument types effectively.
762-800: Effective test coverage forUseEvalHelper.dispatch.The test case ensures correct behavior for calling
denops.dispatch.
802-869: Effective test coverage forUseEvalHelperproperties.The test cases ensure correct behavior for accessing various
UseEvalHelperproperties.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
+ Coverage 82.61% 84.42% +1.80%
==========================================
Files 57 63 +6
Lines 2979 3293 +314
Branches 251 291 +40
==========================================
+ Hits 2461 2780 +319
+ Misses 516 511 -5
Partials 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- eval/expression.ts (1 hunks)
- eval/string.ts (1 hunks)
- eval/use_eval.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- eval/use_eval.ts
Additional context used
Biome
eval/expression.ts
[error] 101-101: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
eval/string.ts
[error] 134-134: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (6)
eval/expression.ts (3)
87-93: LGTM! TestisExpressionfor edge cases.The
isExpressionfunction is correctly implemented. Consider testing it with various edge cases to ensure robustness.
98-102: Refactor: Avoid usingthisin a static context.Using
thisin a static context can be confusing. Use the class nameExpressionImplinstead.- this.prototype[Symbol.toStringTag] = "Expression"; + ExpressionImpl.prototype[Symbol.toStringTag] = "Expression";Tools
Biome
[error] 101-101: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
66-73: LGTM! Verifystringifyhandles all edge cases.The
exprfunction is well-structured. Ensure thatstringifycorrectly handles all possible substitution values.Verification successful
stringifyfunction handles edge cases effectively.The
stringifyfunction is thoroughly tested across various scenarios, ensuring robust handling of different data types and edge cases.
- Located in
eval/stringify.ts.- Extensive tests are in
eval/stringify_test.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `stringify` to ensure it handles all edge cases. # Test: Search for the `stringify` function definition. ast-grep --lang typescript --pattern $'function stringify($_) { $$$ }'Length of output: 73
Script:
#!/bin/bash # Description: Broaden the search for the `stringify` function definition. # Try searching for `stringify` defined as a constant or exported function. rg 'stringify' -A 5 --type=typescriptLength of output: 72
Script:
#!/bin/bash # Description: Correct the search for the `stringify` function definition. # Correct the file type to `ts` for TypeScript files. rg 'stringify' -A 5 --type=tsLength of output: 24870
eval/string.ts (3)
99-105: LGTM!The
rawStringfunction is well-structured and correctly constructs aRawString.
120-126: LGTM! TestisRawStringfor edge cases.The
isRawStringfunction is correctly implemented. Consider testing it with various edge cases to ensure robustness.
131-135: Refactor: Avoid usingthisin a static context.Using
thisin a static context can be confusing. Use the class nameRawStringImplinstead.- this.prototype[Symbol.toStringTag] = "RawString"; + RawStringImpl.prototype[Symbol.toStringTag] = "RawString";Tools
Biome
[error] 134-134: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- eval/stringify_test.ts (1 hunks)
Additional context used
Biome
eval/stringify_test.ts
[error] 229-229: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (2)
eval/stringify_test.ts (2)
11-300: Comprehensive test coverage forstringify().The test cases for the
stringify()function are well-structured and cover a wide range of data types and scenarios, ensuring robust validation of the function's behavior.Tools
Biome
[error] 229-229: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
303-340: Effective use of parameterized testing intest().The test case uses parameterized testing effectively to validate
stringify()values, ensuring compatibility with Vim evaluations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- eval/expression_test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- eval/expression_test.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- deno.jsonc (3 hunks)
- eval/expression.ts (1 hunks)
- eval/string.ts (1 hunks)
- eval/stringify.ts (1 hunks)
- eval/use_eval.ts (1 hunks)
- helper/expr_string.ts (4 hunks)
- lambda/mod.ts (6 hunks)
Additional comments not posted (29)
eval/expression.ts (5)
26-26: Type Definition is Correct.The
Expressiontype is correctly defined as a string with additional properties fromExpressionProps.
28-35: Interface Definition is Correct.The
ExpressionPropsinterface is well-defined, extendingVimEvaluatableand providing necessary methods.
66-73: Function Implementation is Correct.The
exprfunction correctly creates anExpressionfrom a template string and substitutions.
87-93: Function Implementation is Correct.The
isExpressionfunction correctly checks if a value is anExpressionusingisIntersectionOf.
98-113: Class Implementation is Correct.The
ExpressionImplclass correctly extendsStringand implementsExpressionProps, providing necessary methods.eval/use_eval.ts (4)
25-31: Function Implementation is Correct.The
useEvalfunction is correctly implemented, allowing the use of expressions within a Denops context.
35-50: Function Implementation is Correct.The
ensurePrerequisitesfunction is correctly implemented, setting up necessary Vim script functions.
52-55: Function Implementation is Correct.The
trimEndOfArgsfunction correctly removes undefined values from the end of an array.
57-123: Class Implementation is Correct.The
UseEvalHelperclass correctly extends Denops, providing additional methods for expression evaluation.deno.jsonc (3)
11-11: Export Path Addition is Correct.The export path for the
./evalmodule is correctly added.
45-45: Version Update is Correct.The version update for
@core/unknownutilto^4.3.0is appropriate and consistent with maintaining up-to-date dependencies.
51-51: New Dependency Addition is Correct.The new dependency
@nick/disposewith version^1.1.0is appropriately added, likely for resource management or cleanup.eval/string.ts (4)
32-32: Type definition is correct.The
RawStringtype correctly combinesstringwithRawStringProps.
99-105: Function implementation is correct.The
rawStringfunction correctly processes the template and substitutions to return aRawString.
120-126: Function implementation is correct.The
isRawStringfunction correctly uses intersection checks to determine if a value is aRawString.
131-151: Class implementation is correct.The
RawStringImplclass correctly extendsStringand implementsRawStringProps, providing necessary methods and properties.eval/stringify.ts (5)
64-132: Function implementation is correct.The
stringifyfunction effectively converts JavaScript values to Vim-evaluable strings, handling various data types and circular references.
134-136: Function implementation is correct.The
isIgnoreRecordValuefunction correctly identifies ignorable values in a record.
138-140: Constants are correctly defined.The constants for checking specific object types are correctly defined using
isInstanceOf.
142-148: Function implementation is correct.The
toVimExpressionfunction correctly converts aVimEvaluatableto a Vim expression string, ensuring type safety.
153-165: Function implementation is correct.The
toBlobLiteralfunction efficiently converts anArrayBufferto a Vim blob literal using a cached array of hex strings.lambda/mod.ts (5)
Line range hint
153-198: Interface changes are correct.The
Lambdainterface changes enhance type safety and align with the new evaluation context.
254-279: Function changes are correct.The
addfunction changes improve flexibility and type safety in handling function calls.
283-283: Constant definition is correct.The
VIM_REQUEST_FLAGprovides a clear and standardized approach to handling request flags.
285-289: Type definition is correct.The
FnWrapperArgstype aligns with the intended use and enhances type safety.
291-295: Function implementation is correct.The
isFnWrapperArgsfunction correctly uses tuple checks to ensure argument structure.helper/expr_string.ts (3)
48-49: Deprecation notice forExprStringis clear.The deprecation notice provides clear guidance on using
rawStringandRawStringas alternatives.
101-101: Deprecation notice forexprQuoteis clear.The deprecation notice correctly advises using
rawStringas an alternative.
126-127: Deprecation notice forisExprStringis clear.The deprecation notice provides clear guidance on using
rawStringandisRawStringas alternatives.
|
@lambdalisue Would it be better to export it separately like |
which "it"? |
Should I export |
|
If there is demand, then it should be done. If you can't imagine any demand, then at the very least, it should be treated as private for now. This decision can be based on the implementer's 'intuition.' For example, something like |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- deno.jsonc (3 hunks)
- eval/expression.ts (1 hunks)
- eval/string.ts (1 hunks)
- eval/stringify.ts (1 hunks)
- eval/use_eval.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- eval/use_eval.ts
Files skipped from review as they are similar to previous changes (4)
- deno.jsonc
- eval/expression.ts
- eval/string.ts
- eval/stringify.ts
jsr.io does not support `{@linkcode class#member}` style.
#260 (comment)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- helper/expr_string.ts (4 hunks)
- lambda/mod.ts (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- helper/expr_string.ts
- lambda/mod.ts
This is implements of #243.
New feature
evalmodule.exprtagged template string function returnsExpression.rawStringtagged template string function returnsRawString.exprQuoteandExprStringinhelper/expr_stringmodule.stringify()function is similar toJSON.stringify()but it serializes values into a string that can be evaluated in Vim.Expression,RawStringor any JavaScript values.ExprStringfor backward compatibility.useEvalfunction is similar touseExprString.Expression,RawString.ExprStringfor backward compatibility.lambdamodule.Lambda.notify()orLambda.request()allows arguments to beExpressionorRawString.Lambda.request()allows return value to beExpressionorRawString.Lambda.notify()orLambda.request()arguments. (e.g.boolean)helper/keymapmodule.send()allows arguments to beRawString.Mark deprecated
helper/expr_stringmodule as deprecated.Summary by CodeRabbit
New Features
stringifyutility to convert values into a Vim-compatible string format.useEvalfunction for enhanced expression evaluation in asynchronous blocks.Enhancements
RawString.Deprecations
Tests
stringifyfunctionality and expression handling, ensuring robust validation of output across various input scenarios.