refactored tryPost #1121
refactored tryPost #1121
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR does an excellent job of refactoring the tryPost functionality into a more modular, service-based architecture. The separation of concerns is well-implemented, making the code more maintainable and testable. I've identified a few minor improvements that could enhance the robustness of the implementation.
…re reliable with cloning before saving
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
| return this; | ||
| } | ||
|
|
||
| private isComplete(obj: unknown): obj is LogObject { |
There was a problem hiding this comment.
This feels pretty redundant. We are first doing typeof checks for all the fields and then we are also running zod schema validation. Can we remove this isComplete method?
| import { Context } from 'hono'; | ||
| import { RequestContext } from './requestContext'; | ||
|
|
||
| export class PreRequestValidatorService { |
There was a problem hiding this comment.
Hey, this also feels redundant. preRequestValidator is something that we accept in the context and it can be set in other middleware.
Creating a class which internally is just making a function call is redundant because I cannot think of any scope of extra functionalities that we will be able to abstract in future with this.
| import { RequestContext } from './requestContext'; | ||
| import { AllHookResults } from '../../middlewares/hooks/types'; | ||
|
|
||
| export class HooksService { |
There was a problem hiding this comment.
Isn't it better to just add the new methods to the existing class itself? areSyncHooksAvailable, hasResults and hasFailedHooks can be first class citizens instead of this new class taking the decisions.
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Description
Cleaning up handlerUtils in preparation to add more features here.
Type of Change
How Has This Been Tested?
Checklist