Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #14 #46

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Fix issue #14 #46

merged 1 commit into from
Feb 12, 2024

Conversation

odbz
Copy link
Contributor

@odbz odbz commented Feb 8, 2024

No description provided.

@odbz odbz requested a review from SergeySukhanov February 8, 2024 15:22
@@ -20,6 +20,7 @@ export enum Status {
IN_PROGRESS = "IN_PROGRESS",
IN_PROGRESS_RELOAD = "IN_PROGRESS_RELOAD",
SUCCESS = "SUCCESS",
DELETE_SUCCESS = "DELETE_SUCCESS",
ERROR = "ERROR",
DISABLED = "DISABLED",
} No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I cannot see the actual code patch you mentioned. However, based on the information provided, it appears that you are modifying an enum called Status. You have added a new enum value called DELETE_SUCCESS to the enum.

For a brief code review, here are a few suggestions:

  1. Make sure to add a newline at the end of the file. This is not a critical issue but helps with readability and avoids any warnings or errors from certain linters or compilers.
  2. Naming conventions: Enums are typically named in TitleCase, so you might want to consider renaming the enum from Status to StatusCode or something similar to follow common naming conventions.
  3. Consider documenting the purpose and usage of each enum value, including the newly added DELETE_SUCCESS. This can provide additional clarity for developers who use this enum.

Without the actual code implementation, it's challenging to identify potential bug risks or further improvements. If you provide the relevant code, I'll be happy to assist you with a more extensive review.

@@ -55,7 +55,7 @@ export const reducer = <T extends object>(
draft.status = Status.IN_PROGRESS;
break;
case EntityActions.DELETE_ENTITY_SUCCESS:
draft.status = Status.SUCCESS;
draft.status = Status.DELETE_SUCCESS;
break;
// case EntityActions.DELETE_ENTITY_ERROR:
// draft.status = Status.ERROR;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the given code patch, here are a few observations and suggestions for improvement:

  1. In line 55, export const reducer = <T extends object>( seems incomplete. It should be followed by the function body or other required parameters.

  2. It is generally good practice to include a default case in switch statements to handle unexpected actions. Consider adding a default case to cover any unhandled actions.

  3. In line 58, changing draft.status to Status.DELETE_SUCCESS after the DELETE_ENTITY_SUCCESS action seems reasonable if you have defined Status.DELETE_SUCCESS as a valid status in your application.

  4. The commented-out cases for DELETE_ENTITY_ERROR could be removed to keep the code clean and concise. If you need to handle errors, consider uncommenting those lines and handling the error status appropriately in the reducer.

  5. It would be helpful to review the overall flow of the reducer and ensure it handles all the necessary actions and updates the state correctly.

Remember that without seeing the complete code and its surrounding context, it's difficult to provide a thorough review.

@OHDSI OHDSI deleted a comment from github-actions bot Feb 8, 2024
@odbz odbz merged commit 9357dbe into master Feb 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants