Refactor item group handling and improve code quality #597
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d improve validation error handling; update import paths for consistency
…odule; update import paths in MealEditView and RecipeEditView
…rvice; update MealEditView and RecipeEditView to use the new service
…nvertToGroups function in MealEditView and RecipeEditView
…y and maintainability
…lines and examples for improved clarity and maintainability
…ctions for improved maintainability
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the item group handling by migrating legacy utilities into dedicated modules and deprecating the legacy editor pattern in favor of pure functions, while also updating documentation to clarify the new guidelines.
- Migrated functions (e.g. convertToGroups, isRecipedGroupUpToDate) from legacy utils to dedicated modules.
- Added TODOs to deprecate legacy Editor patterns in MealEditor, ItemGroupEditor, DayDietEditor, and RecipeEditor.
- Updated documentation (CODESTYLE_GUIDE.md and ARCHITECTURE_GUIDE.md) and package version to reflect the new practices.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/sections/meal/components/MealEditView.tsx | Updated import for convertToGroups and marked legacy MealEditor deprecation. |
| src/sections/item-group/components/ItemGroupView.tsx | Updated import for isRecipedGroupUpToDate to use the new module. |
| src/sections/item-group/components/ItemGroupEditModal.tsx | Updated import for isRecipedGroupUpToDate and marked deprecated ItemGroupEditor usage. |
| src/modules/diet/meal/application/meal.ts | Added a TODO comment regarding the deprecation of DayDietEditor. |
| src/modules/diet/item-group/domain/itemGroup.ts | Added new implementation for isRecipedGroupUpToDate with error handling via throw. |
| src/modules/diet/item-group/application/itemGroupService.ts | Introduced convertToGroups with robust type checking and conversion logic. |
| src/legacy/utils/macroMath.ts | Marked deprecated calcGroupMacros for future removal. |
| src/legacy/utils/groupUtils.ts | Removed legacy group utility functions. |
| Multiple legacy files (e.g. recipeEditor.ts, mealEditor.ts, itemGroupEditor.ts, dayDietEditor.ts, itemEditor.ts) | Deprecated legacy editor patterns with TODO comments and guidance in tests. |
| package.json | Version updated to reflect the development branch and issue reference. |
| CODESTYLE_GUIDE.md & ARCHITECTURE_GUIDE.md | Updated documentation to outline current coding practices and migration guidelines. |
Comments suppressed due to low confidence (1)
src/modules/diet/item-group/application/itemGroupService.ts:49
- The type guard functions (isRecipe, hasGroups, isItem, isItemGroup) use basic checks; consider enhancing them to include more robust validations as outlined in the CODESTYLE_GUIDE.md to improve type safety.
throw new Error(`Unsupported convertible type: ${getTypeDescription(convertible)}`)
…and cross-references for improved clarity and consistency
…es for improved error management
…in convertToGroups function
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the item group handling logic by migrating key functions to a dedicated domain module, deprecating legacy editor patterns in favor of pure functions, and updating documentation to reflect new standards.
- Migrates the isRecipedGroupUpToDate function and its related utilities to the new domain module.
- Deprecates and adds TODO comments for legacy editor patterns across multiple modules.
- Updates documentation and versioning (e.g., package.json and CODESTYLE_GUIDE.md) in support of improved code quality.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sections/item-group/components/ItemGroupView.tsx | Updates import for isRecipedGroupUpToDate to the new domain module. |
| src/sections/item-group/components/ItemGroupEditModal.tsx | Migrates isRecipedGroupUpToDate import and adds a TODO comment for deprecated editor usage. |
| src/modules/diet/meal/application/meal.ts | Adds a TODO comment regarding removal of deprecated DayDietEditor usage. |
| src/modules/diet/item-group/domain/itemGroup.ts | Implements the migrated isRecipedGroupUpToDate with error handling improvements. |
| src/modules/diet/item-group/application/itemGroupService.ts | Adds a pure function to convert group-like objects using multiple type guards. |
| src/modules/diet/item-group/application/itemGroup.ts | Adds a TODO comment regarding removal of deprecated DayDietEditor usage. |
| src/legacy/utils/macroMath.ts | Adds a TODO comment to remove deprecated calcGroupMacros. |
| src/legacy/utils/groupUtils.ts | Deletes legacy group utilities as they have been migrated. |
| src/legacy/utils/data/* | Adds TODO comments deprecating legacy Editor patterns (RecipeEditor, MealEditor, ItemEditor, etc.). |
| package.json | Updates versioning to reflect pre-release state for development. |
| CODESTYLE_GUIDE.md, ARCHITECTURE_GUIDE.md, .vscode/settings.json | Updates and provisions for new documentation and configuration guidelines. |
…ecipedGroupUpToDate function
… convertToGroups function
github-actions
bot
added
improvement
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR restructures item group logic into its domain/application modules, deprecates legacy editor patterns in favor of pure functions, and adds updated style/architecture documentation.
- Moved and reimplemented
isRecipedGroupUpToDatein the domain layer. - Introduced
convertToGroupsservice and type guards in the application layer. - Added deprecation TODOs for all legacy
*Editorclasses and removed obsolete utilities. - Published
CODESTYLE_GUIDE.mdandARCHITECTURE_GUIDE.md; updated Copilot settings.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sections/item-group/components/ItemGroupView.tsx | Updated import path for isRecipedGroupUpToDate |
| src/sections/item-group/components/ItemGroupEditModal.tsx | Redirected import and added deprecation TODO for ItemGroupEditor |
| src/modules/diet/meal/application/meal.ts | Added deprecation TODO for DayDietEditor |
| src/modules/diet/item-group/domain/itemGroup.ts | Added isRecipedGroupUpToDate implementation and imports |
| src/modules/diet/item-group/application/itemGroupService.ts | Added convertToGroups service with type guards |
| src/modules/diet/item-group/application/itemGroup.ts | Added deprecation TODO for legacy editor usage |
| src/legacy/utils/macroMath.ts | Marked calcGroupMacros as deprecated |
| src/legacy/utils/groupUtils.ts | Removed legacy groupUtils utility |
| src/legacy/utils/data/{recipeEditor,mealEditor,itemGroupEditor,itemEditor,editor,dayDietEditor}.ts | Added deprecation TODOs for all Editor classes |
| package.json | Bumped version to include origin-dev metadata |
| CODESTYLE_GUIDE.md | Added concrete style & anti-pattern guide |
| ARCHITECTURE_GUIDE.md | Added SolidJS frontend architecture guide |
| .vscode/settings.json | Updated Copilot instruction file paths and commit config |
Comments suppressed due to low confidence (4)
src/modules/diet/item-group/domain/itemGroup.ts:9
- Correct the grammar in the TODO: change "will deprecated" to "will be deprecated".
// TODO: In the future, it seems like discriminated unions will deprecated (https://github.com/colinhacks/zod/issues/2106)
src/modules/diet/item-group/application/itemGroupService.ts:34
- Consider adding unit tests covering each conversion branch (
Array,Recipe,groups,Item,ItemGroup, and the fallback) to ensure all cases behave as expected.
export function convertToGroups(convertible: GroupConvertible): ItemGroup[] {
.vscode/settings.json:4
- The file paths reference
docs/ARCHITECTURE_GUIDE.mdanddocs/CODESTYLE_GUIDE.md, but these guides live at the project root. Update the paths or relocate the files.
"github.copilot.chat.codeGeneration.instructions": [
package.json:5
- The version field includes a leading "v" and a custom metadata string that may not comply with semver; consider removing the leading "v" and ensuring it matches the expected semver format.
"version": "v0.10.0-origin-dev.759+issue.429",
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors item-group logic into dedicated domain and application modules, deprecates legacy Editor patterns in favor of pure functions, and updates project documentation for clearer code quality guidelines.
- Moved and reimplemented
isRecipedGroupUpToDateinitemGroupdomain and added a newconvertToGroupsservice. - Added deprecation TODOs for legacy
*Editorclasses and tests, preparing for their removal. - Introduced or expanded
CODESTYLE_GUIDE.mdandARCHITECTURE_GUIDE.md, and bumped project version.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sections/item-group/components/ItemGroupEditModal.tsx | Updated import for isRecipedGroupUpToDate and marked ItemGroupEditor as deprecated. |
| src/modules/diet/meal/application/meal.ts | Added TODO for removing DayDietEditor usage in meal application. |
| src/modules/diet/item-group/domain/itemGroup.ts | Implemented isRecipedGroupUpToDate and added error handling. |
| src/modules/diet/item-group/application/itemGroupService.ts | Added convertToGroups function with type guards and error handling. |
| CODESTYLE_GUIDE.md | New guide for naming, architecture layers, and anti-patterns. |
| ARCHITECTURE_GUIDE.md | New SolidJS frontend architecture guide. |
| package.json | Bumped version to v0.11.0-dev.13+issue.429. |
Comments suppressed due to low confidence (2)
src/modules/diet/item-group/application/itemGroupService.ts:34
- The new
convertToGroupsfunction lacks unit tests; please add tests covering each input branch (array, recipe, groups object, item, itemGroup, and fallback).
export function convertToGroups(convertible: GroupConvertible): ItemGroup[] {
src/modules/diet/item-group/domain/itemGroup.ts:119
- The newly implemented
isRecipedGroupUpToDatefunction is not covered by tests; consider adding unit tests to validate its behavior and error handling.
export function isRecipedGroupUpToDate(
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migrate functions and classes related to item groups to a dedicated module, enhancing validation error handling. Deprecate outdated editor patterns in favor of pure functions to improve maintainability. Update documentation to provide clearer guidelines for code quality.
Closes #429