refactor: fill in basic types in rules.ts #3365
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
UnderKoen
changed the title
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing type safety through explicit type annotations and structural changes in classes and interfaces. Key updates include refining type definitions in the Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
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: 2
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/types/models/rule.d.ts (1)
4-5: Summary of changes toNewRuleEntityThe modifications to the
NewRuleEntityinterface enhance type safety and standardize logical operations:
- The
stageproperty now uses a union type ('pre' | null | 'post') instead of a generic string, providing clearer intent and restricting possible values.- The
conditionsOpproperty has been updated from'any' | 'and'to'or' | 'and', standardizing logical operators.These changes improve code quality and maintainability. However, they may require updates in other parts of the codebase, including rule processing logic, user interfaces, and potentially data migration for existing rules. Please ensure thorough testing and consider the need for a migration strategy if existing data might be affected.
Consider the following architectural implications:
- Ensure that any serialization/deserialization of rule entities (e.g., for API communication or storage) is updated to handle the new type constraints.
- If there's a database schema for storing rules, it may need to be updated to reflect these changes.
- Review any documentation or API contracts that might be affected by these type changes.
packages/loot-core/src/server/accounts/rules.ts (2)
761-761: Consider adding a type annotation to theobjectparameter inevalConditions.Adding a type annotation enhances type safety and helps catch errors at compile time.
Consider updating the method signature:
- evalConditions(object): boolean { + evalConditions(object: Record<string, unknown>): boolean {
882-882: Suggest typing theobjectparameter ingetApplicableRules.Including a type annotation for
objectimproves type safety and code clarity.Consider changing the method signature to:
- getApplicableRules(object): Set<Rule> { + getApplicableRules(object: Record<string, unknown>): Set<Rule> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
upcoming-release-notes/3365.mdis excluded by!**/*.md
📒 Files selected for processing (4)
- packages/desktop-client/src/components/accounts/Account.tsx (1 hunks)
- packages/loot-core/src/server/accounts/rules.ts (13 hunks)
- packages/loot-core/src/server/accounts/transaction-rules.ts (1 hunks)
- packages/loot-core/src/types/models/rule.d.ts (1 hunks)
🧰 Additional context used
Biome
packages/loot-core/src/server/accounts/rules.ts
[error] 1044-1045: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments not posted (6)
packages/loot-core/src/types/models/rule.d.ts (1)
5-5: Updated logical operators forconditionsOp.The change from
'any' | 'and'to'or' | 'and'standardizes the logical operators used in rule conditions. This is a good improvement for clarity and consistency.However, this change may have some implications:
- Existing rules using 'any' might need to be migrated to use 'or'.
- Rule processing logic may need to be updated to handle 'or' instead of 'any'.
- User interfaces for rule creation/editing might require updates.
To ensure a smooth transition, please run the following verification steps:
#!/bin/bash # Description: Check for usage of 'any' in rule conditions and rule processing logic # Search for 'any' usage in rule conditions echo "Searching for 'any' usage in rule conditions:" rg --type typescript "conditionsOp.*['\"](any|or)['\"]" -A 5 # Search for rule processing logic that might need updates echo "Searching for rule processing logic:" rg --type typescript "(evaluateRule|processRule|applyRule)" -A 10Additionally, please ensure that:
- A migration plan is in place for existing rules using 'any'.
- The UI for rule creation/editing is updated to reflect this change.
- Documentation is updated to explain the use of 'or' instead of 'any' in rule conditions.
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
570-570: Improved type safety with explicitSet<Rule>annotationThe change from
SettoSet<Rule>enhances type safety and code clarity. This explicit type annotation ensures that onlyRuleobjects can be added to the set, which aligns with TypeScript best practices and improves code maintainability.packages/desktop-client/src/components/accounts/Account.tsx (1)
1193-1193: LGTM. Verify impact on rule processing.The change to set
stage: nullfor a new rule looks good. This likely indicates that the rule is in an initial or undefined stage when created.Please verify that setting
stagetonulldoesn't negatively impact rule processing or display in other parts of the application. Run the following script to check for any dependencies on thestageproperty:✅ Verification successful
Verified Change Does Not Negatively Impact Rule Processing
The usage of
stageincludes checks fornullvalues and proper error handling. Settingstage: nullis handled appropriately in both backend and frontend components, ensuring that rule processing and display function as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of 'stage' property in rule-related code rg --type typescript --type javascript 'rule.*\.stage' -g '!*test*'Length of output: 104
Script:
#!/bin/bash # Search for usage of 'stage' property in rule-related code with corrected file types rg --type ts --type js 'rule.*\.stage' -g '!*test*'Length of output: 1003
packages/loot-core/src/server/accounts/rules.ts (3)
26-26: Great use of TypeScript'sassertsfor type narrowing.The addition of
asserts testin the function signature enhances type safety by informing the compiler thattestis asserted to be truthy, which improves code reliability.
729-730: Explicitly typingactionsandconditionsimproves code clarity.Defining
actions: Action[];andconditions: Condition[];makes the code more readable and maintainable by clearly specifying the expected types.
732-733: Confirm handling of optionalidand nullablestageproperties.Making
idoptional (id?: string;) and allowingstageto benull(stage: 'pre' | null | 'post';) is acceptable. Please ensure that all usages of these properties properly handleundefinedandnullvalues to prevent potential runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/rules.ts (1)
Line range hint
890-1020: LGTM with suggestion: Comprehensive type annotations added to utility functions.The addition of type annotations to
computeScore,_rankRules,rankRules, andmigrateIdsfunctions significantly enhances type safety and code clarity. These annotations will help prevent type-related errors and improve code maintainability.However, there's a minor improvement that can be made:
In the
iterateIdsfunction, consider replacing thevoid | booleanreturn type of the callback function withboolean | undefined. This change addresses the static analysis hint and improves type clarity:export function iterateIds( rules: Rule[], fieldName: string, - func: (rule: Rule, id: string) => void | boolean, + func: (rule: Rule, id: string) => boolean | undefined, ): void { // ... rest of the function }This change maintains the same functionality while avoiding the use of
voidin a union type, which can be confusing and is flagged by static analysis tools.🧰 Tools
🪛 Biome
[error] 1019-1020: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/loot-core/src/server/accounts/rules.ts (13 hunks)
- packages/loot-core/src/server/accounts/transaction-rules.ts (1 hunks)
- packages/loot-core/src/types/models/rule.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loot-core/src/server/accounts/transaction-rules.ts
- packages/loot-core/src/types/models/rule.d.ts
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: UnderKoen PR: actualbudget/actual#3365 File: packages/loot-core/src/types/models/rule.d.ts:4-4 Timestamp: 2024-10-02T08:45:11.136Z Learning: In `packages/loot-core/src/server/accounts/transaction-rules.ts`, the `stage` property can have legacy values `'cleanup'` and `'modify'`, which are converted to `'pre'`. The type remains `string` to accommodate these values and ensure correct usage.
🪛 Biome
packages/loot-core/src/server/accounts/rules.ts
[error] 1019-1020: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (7)
packages/loot-core/src/server/accounts/rules.ts (7)
27-31: LGTM: Improved type safety with imports and assert function update.The addition of
RuleConditionEntityandRuleEntityimports, along with the type annotations for theassertfunction, enhances the overall type safety of the code. These changes are well-implemented and align with best practices for TypeScript.
706-710: LGTM: Appropriate type annotations added to Rule class properties.The type annotations added to the
Ruleclass properties enhance type safety and clarity. Thestageproperty type ('pre' | null | 'post') correctly aligns with the retrieved learning about legacy values, which mentions that 'cleanup' and 'modify' are converted to 'pre'. This implementation ensures correct usage while accommodating legacy values.
720-726: LGTM: Constructor parameters properly typed and aligned with class properties.The type annotations added to the constructor parameters are consistent with the class property types, improving type safety. Making the
stageparameter optional with a default value ofnullis a good practice, allowing flexibility in rule creation while maintaining type consistency.
736-736: LGTM: Appropriate return type added to evalConditions method.Adding the
booleanreturn type to theevalConditionsmethod improves type safety and code clarity. This change helps developers understand the expected output of the method without needing to examine its implementation.
773-777: LGTM: Proper return types added to getId and serialize methods.The addition of return type annotations to the
getIdandserializemethods enhances type safety and code clarity:
- The
string | undefinedreturn type forgetIdcorrectly reflects that theidproperty is optional.- The
RuleEntityreturn type forserializealigns with the imported type, ensuring consistency between the serialized output and the expected shape of a rule entity.These changes improve the overall type correctness of the
Ruleclass.
789-791: LGTM: Appropriate type annotations added to RuleIndexer class properties.The type annotations added to the
RuleIndexerclass properties enhance type safety and code clarity:
field: stringcorrectly specifies that the field is a string.method?: stringindicates that the method is an optional string property.rules: Map<string, Set<Rule>>provides a clear structure for the rules storage.These type annotations will help prevent type-related errors and improve code maintainability.
Line range hint
799-857: LGTM: Comprehensive type annotations added to RuleIndexer methods.The addition of type annotations to the
RuleIndexerclass methods significantly enhances type safety and code clarity:
getIndex,getIndexForValue, andgetKeymethods now have clear input and output types.- The use of
unknownfor generic input in methods likegetIndexForValueandgetKeyis appropriate, allowing for type-safe handling of various inputs.getIndexes,index, andremovemethods now clearly specify that they work withRuleobjects.- The
getApplicableRulesmethod's return typeSet<Rule>clearly indicates the expected output.These type annotations will help catch potential type-related errors early and improve the overall robustness of the
RuleIndexerclass.
…ules # Conflicts: # packages/loot-core/src/server/accounts/rules.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/rules.ts (1)
Line range hint
879-1110: LGTM: Improved type safety for RuleIndexer and utility functions.The addition of explicit type annotations to the
RuleIndexerclass and utility functions significantly enhances the overall type safety and clarity of the code.However, there's a minor improvement we can make:
In the
iterateIdsfunction (lines 1109-1110), consider replacingvoidwithundefinedin the union type of thefuncparameter. This change addresses a potential confusion in the type definition:func: (rule: Rule, id: string) => boolean | undefined,This modification aligns with best practices for union types in TypeScript.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/loot-core/src/server/accounts/rules.ts (14 hunks)
- packages/loot-core/src/types/models/rule.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/models/rule.d.ts
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: UnderKoen PR: actualbudget/actual#3365 File: packages/loot-core/src/types/models/rule.d.ts:4-4 Timestamp: 2024-10-08T15:46:15.739Z Learning: In `packages/loot-core/src/server/accounts/transaction-rules.ts`, the `stage` property can have legacy values `'cleanup'` and `'modify'`, which are converted to `'pre'`. The type remains `string` to accommodate these values and ensure correct usage.Learnt from: UnderKoen PR: actualbudget/actual#3365 File: packages/loot-core/src/types/models/rule.d.ts:4-4 Timestamp: 2024-10-02T08:45:11.136Z Learning: In `packages/loot-core/src/server/accounts/transaction-rules.ts`, the `stage` property can have legacy values `'cleanup'` and `'modify'`, which are converted to `'pre'`. The type remains `string` to accommodate these values and ensure correct usage.
🪛 Biome
packages/loot-core/src/server/accounts/rules.ts
[error] 1109-1110: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (7)
packages/loot-core/src/server/accounts/rules.ts (7)
30-31: LGTM: New imports added for type definitions.The addition of
RuleConditionEntityandRuleEntityimports from the models file is appropriate for enhancing type safety in this module.
90-93: LGTM: Improved type safety for assert function.The
assertfunction now has explicit parameter types and uses theassertskeyword in its return type, which is the correct approach for assertion functions in TypeScript.
796-800: LGTM: Improved type definitions for Rule class properties.The explicit typing of the
Ruleclass properties enhances type safety. Thestageproperty type ('pre' | null | 'post') is consistent with the retrieved learnings about legacy values being converted to 'pre'.
810-816: LGTM: Constructor parameters properly typed.The
Ruleconstructor parameters are now properly typed, maintaining consistency with the property definitions. The optionalstageparameter with a default value ofnullis well-implemented.
Line range hint
826-863: LGTM: Enhanced type safety for Rule class methods.The addition of return types and the use of generics in the
Ruleclass methods significantly improves type safety and code clarity. TheexecActionsmethod's generic implementation is particularly well done, allowing for flexible usage while maintaining strong typing.
Line range hint
867-876: LGTM: Proper return type for serialize method.The
serializemethod now correctly specifies its return type asRuleEntity, which enhances type safety when working with serialized rule data.
Line range hint
1-1160: Excellent work on enhancing type safety throughout the file!This refactoring significantly improves the type definitions in
rules.ts. The changes consistently apply TypeScript best practices, enhancing code clarity, maintainability, and reducing the potential for type-related errors. The explicit typing of class properties, method parameters, and return types will greatly aid in catching potential issues early in the development process.The only minor suggestion is the adjustment to the
iterateIdsfunction parameter type, which has been noted in a previous comment.Overall, this is a very well-executed refactoring that substantially improves the codebase.
No description provided.