👍 implement options method with class #266
Conversation
WalkthroughThe changes involve a significant refactor of option handling in a Denops environment. Key modifications include the removal of multiple functions related to option formatting, the introduction of a new utility class structure for managing options, and the instantiation of options using specific classes such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormatFunction
participant OptionConstructor
User->>FormatFunction: Request to format options
FormatFunction->>OptionConstructor: Call getOptionConstructor()
OptionConstructor-->>FormatFunction: Return new option instance
FormatFunction-->>User: Return formatted options
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: 0
Outside diff range, codebase verification and nitpick comments (2)
.scripts/gen-option/format.ts (1)
39-51: Refactor approved, but ensure error handling.The
getOptionConstructorfunction introduces a modular approach by using specific classes for different option types. Ensure that the calling code properly handles the potential error thrown by the default case to avoid runtime exceptions.The code changes are approved.
Consider adding error handling in the calling code to manage the exceptions thrown by this function effectively.
option/_utils.ts (1)
13-35: Class implementation approved, suggest enhancing error handling.The
OptionImplclass provides a robust structure for option handling with type safety through coercion functions. Consider enhancing error handling for asynchronous operations to manage potential exceptions more effectively.The code changes are approved.
Consider implementing try-catch blocks around asynchronous operations to handle exceptions gracefully.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .scripts/gen-option/format.ts (3 hunks)
- option/_utils.ts (1 hunks)
- option/nvim/_generated.ts (15 hunks)
Additional comments not posted (20)
.scripts/gen-option/format.ts (4)
8-12: LGTM!The
formatDocsfunction correctly formats documentation strings.The code changes are approved.
Line range hint
25-31: LGTM!The
getOptionTypeNamefunction correctly determines the type name based on the option's scope.The code changes are approved.
56-60: Refactor approved for improved maintainability.The
formatfunction effectively integrates the new changes, using flat mapping to simplify the generation of formatted lines. This enhances maintainability and clarity.The code changes are approved.
21-23: Refactor approved, but verify the translation dictionary.The refactoring of
formatOptionto usegetOptionConstructorsimplifies the code. However, ensure that thetranslatedictionary is correctly maintained and covers all necessary cases.The code changes are approved.
Run the following script to verify the translation dictionary usage:
option/_utils.ts (2)
82-86: Specialization for boolean options approved.The
BooleanOptionclass is correctly implemented, extendingOptionImplwith appropriate settings for boolean options.The code changes are approved.
88-98: Specializations for number and string options approved.The
NumberOptionandStringOptionclasses are correctly implemented, extendingOptionImplwith appropriate settings for their respective types.The code changes are approved.
option/nvim/_generated.ts (14)
16-16: Refactor toNumberOptionforchannelapproved.The implementation using
NumberOptionsimplifies handling and is correctly set up for a local option of type number.The code changes are approved.
35-35: Refactor toStringOptionforinccommandapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
60-62: Refactor toStringOptionformousescrollapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
80-80: Refactor toNumberOptionforpumblendapproved.The implementation using
NumberOptionsimplifies handling and is correctly set up for a global option of type number.The code changes are approved.
116-118: Refactor toStringOptionforredrawdebugapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
128-128: Refactor toNumberOptionforscrollbackapproved.The implementation using
NumberOptionsimplifies handling and is correctly set up for a local option of type number.The code changes are approved.
247-247: Refactor toStringOptionforshadaapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
259-259: Refactor toStringOptionforshadafileapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
323-325: Refactor toStringOptionforstatuscolumnapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a local option of type string.The code changes are approved.
349-351: Refactor toStringOptionfortermpastefilterapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global option of type string.The code changes are approved.
361-361: Refactor toBooleanOptionfortermsyncapproved.The implementation using
BooleanOptionsimplifies handling and is correctly set up for a global option of type boolean.The code changes are approved.
380-380: Refactor toStringOptionforwinbarapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a global or local option of type string.The code changes are approved.
391-391: Refactor toNumberOptionforwinblendapproved.The implementation using
NumberOptionsimplifies handling and is correctly set up for a local option of type number.The code changes are approved.
414-416: Refactor toStringOptionforwinhighlightapproved.The implementation using
StringOptionsimplifies handling and is correctly set up for a local option of type string.The code changes are approved.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
+ Coverage 84.23% 84.63% +0.39%
==========================================
Files 62 63 +1
Lines 3292 3377 +85
Branches 291 291
==========================================
+ Hits 2773 2858 +85
Misses 517 517
Partials 2 2 ☔ View full report in Codecov by Sentry. |
|
We should avoid breaking changes unless there are significant benefits for both developers and users. However, I believe this PR does not offer enough value for users. Pros
Cons
Therefore, I think we should not accept this PR. Did I misunderstand your perspective? |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .scripts/gen-option/format.ts (3 hunks)
- option/_utils.ts (1 hunks)
- option/nvim/_generated.ts (15 hunks)
Files skipped from review as they are similar to previous changes (2)
- option/_utils.ts
- option/nvim/_generated.ts
Additional comments not posted (2)
.scripts/gen-option/format.ts (2)
39-51: Implement a switch-case for option type instantiation.This new function,
getOptionConstructor, uses a switch-case to instantiate different types of options based on thetypeparameter. This approach is clean and straightforward, making it easy to understand and maintain.However, the use of a
nevertype in line 48 as a fallback for unknown types is a good practice to ensure type safety. This ensures that the function will fail at runtime if an unsupported type is passed, which can help catch errors early during development.The implementation here is robust and adheres to good practices in TypeScript for ensuring type safety and clarity.
56-60: Update imports to reflect new utility usage.The import statements have been updated to reflect the new structure and dependencies, specifically importing the
BooleanOption,NumberOption, andStringOptionfrom a utility module. This change supports the new object-oriented approach to handling options.The changes to the imports are appropriate and align with the new architectural approach. Ensure that the utility module is correctly implemented and accessible from this path to avoid module resolution errors.
| `export const ${name}: ${getOptionTypeName(scope, type)} = ${ | ||
| getOptionConstructor(name, type) | ||
| };`, |
There was a problem hiding this comment.
Refactor to use dynamic option instantiation.
The refactoring to use getOptionConstructor for creating option instances simplifies the code by removing multiple conditional checks and function calls that were previously used for this purpose. This change should enhance maintainability and clarity as it centralizes the instantiation logic.
However, as noted in the PR comments, this change introduces a breaking change by altering how options are accessed and instantiated. It is crucial to ensure that this new method does not lead to TypeError when properties are accessed on undefined, which has been a concern with the new implementation.
Ensure thorough testing and documentation are provided to guide developers on the new usage patterns to prevent runtime errors.
There are two problems:
All methods exist for all options (global or local), but not for the public types. import * as op from "jsr:@denops/std/option";
declare const denops: Denops;
op.ambiwidth.getLocal(denops); // TS2551 [ERROR]: Property 'getLocal' does not exist on type 'GlobalOption<string>'.
(op.ambiwidth as any).getLocal(denops); // No error. |
Oops. I didn't notice that.
Ah, I see. Then it's OK. So this change is breaking ONLY for case 3 right? Case 1import * as op from "jsr:@denops/std/option";
console.log(await op.ambiwidth.get(denops));Case 2import { ambiwidth } from "jsr:@denops/std/option";
console.log(await ambiwidth.get(denops));Case 3import { ambiwidth } from "jsr:@denops/std/option";
const getAmbiwidth = ambiwidth.get;
//const getAmbiwidth = ambiwidth.get.bind(ambiwidth); // Must be like this
console.log(await getAmbiwidth(denops)); |
Yes, ONLY for case 3 has the breaking change. |
option/**)Breaking
This change breaks codes like below.
But I think these can be included in the specifications as unexpected uses.:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes