Docblock Updates and General Improvements #4
Conversation
callyall
commented
Apr 5, 2024
callyall
commented
- Updated inaccurate and added missing docblocks
- Simplified the overall structure
- Deprecated the "with" methods
- Updated inaccurate and added missing docblocks - Simplified the overall structure - Deprecated the "with" methods
|
|
||
| public function __construct(Module $module) | ||
| /** @param Array<ProcedureRoute>|Generator<ProcedureRoute> $_routes */ | ||
| public function __construct(protected array|Generator $_routes) |
There was a problem hiding this comment.
This should stay as module, otherwise its a breaking change.
There was a problem hiding this comment.
Looking at the code base this looks like an object that is used internally and I've already updated the reference(https://github.com/cubex/api-foundation/pull/4/files#diff-d08ab29dc7d96cd6c5d5f15f1c75366b9f4167b82a94951330adda2d7bac1ec6R30)?
| * @param int $code | ||
| * @param Throwable|null $previous | ||
| */ | ||
| public function __construct(string $type, string $message = "", int $code = 0, ?Throwable $previous = null) |
There was a problem hiding this comment.
This doesn't follow the constructor for Exception
There was a problem hiding this comment.
As we are looking to add a type I don't see why we want it to follow the exception constructor.
| { | ||
| $payload->fromContext($c); | ||
| } | ||
| throw new Exception("Unable to handle procedure: execute missing"); |
| return JsonResponse::create($response); | ||
| if (!$plClass) | ||
| { | ||
| return JsonResponse::create($procedure->execute()); |
There was a problem hiding this comment.
We should avoid creating the JsonResponse twice here.
Its preferred to create the final response in a single line for improved refactoring or modification in the future.
Another option to would be to have a method to convert the response to the JsonResponse. This would give us the option to return as text/xml for different clients, or endpoints.
There was a problem hiding this comment.
Having an else statement increases code complexity. An early return does look like the better option here.
If we need to modify any of this logic we can override the method.
| public function complete(Context $context) | ||
| { | ||
| $context->meta()->set('api.module', get_class($this->_module)); | ||
| $context->meta()->set('api.module', $this->_moduleClassName); |
There was a problem hiding this comment.
As class names are not case sensitive, sticking with get_class here is safer
