Refactors #29
Refactors #29
Conversation
…ser require flightphp/tracy-extensions:^0.2.7 rector/rector:^2.1 --dev -W
There was a problem hiding this comment.
Pull Request Overview
This pull request updates package dependencies to newer versions and removes a default parameter assignment. The changes focus on keeping the codebase current with the latest versions of Flight PHP packages and development tools.
- Updated core Flight PHP packages to newer minor/patch versions
- Updated development dependencies to newer versions
- Removed default empty array parameter in a method signature
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| composer.json | Updates package versions for core Flight PHP dependencies and dev tools |
| app/utils/Text.php | Removes default parameter assignment from method signature |
app/utils/Text.php
Outdated
| * @return string | ||
| */ | ||
| public static function generateAndConvertHeaderListFromHtml(string $markdown_html, array &$heading_data = [], string $section_file_path): string { | ||
| public static function generateAndConvertHeaderListFromHtml(string $markdown_html, array &$heading_data, string $section_file_path): string { |
There was a problem hiding this comment.
Removing the default value = [] from the &$heading_data parameter is a breaking change. Callers that previously relied on this parameter being optional will now get a fatal error. Consider keeping the default value or updating all call sites to explicitly pass an array.
| public static function generateAndConvertHeaderListFromHtml(string $markdown_html, array &$heading_data, string $section_file_path): string { | |
| public static function generateAndConvertHeaderListFromHtml(string $markdown_html, array &$heading_data = [], string $section_file_path): string { |
Copilot uses AI. Check for mistakes.
…r.php translate_content.php
fadrian06
force-pushed
the
refactors
branch
from
99eea6f to
00f736c
Compare
There was a problem hiding this comment.
So overall I like that the syntax is being cleaned up and it'll look prettier in the end. The thing I have a hard time with is why everything moved to Flight:: from $app. It will make unit testing harder and less straightforward because on every test run you have to clear Flight globals so that the next test isn't contaminated against the previous test.
The other thing that was a little hard for me was that you grouped together a lot of (good) changes but they are now all in one massive pull request. If you look at it from my perspective, I'm trying to do a code review and I don't really know what you changed. But because there was spacing changes, and name changes and variable changes and then legitimate code changes, it's really hard for me to know what actually changed because huge blocks of code were deleted and then huge blocks of code were added. I had to scroll up, then down, then up and down to try and understand what changes were even happening if any changes happened at all! You don't have to change this PR because of that, but just for the future, it's helpful to have very small PRs that are only doing one "thing". Like I would have a PR for code beautifying, one PR for changing everything to Flight (even though I don't love that), one PR for putting everything in the container, etc. Make sense?
| throw new Exception('Swoole is not installed. Please install Swoole or define the NOT_SWOOLE constant in public/index.php to run without Swoole.'); | ||
| } | ||
|
|
||
| call_user_func('Swoole\Runtime', 'enableCoroutine'); |
There was a problem hiding this comment.
Why is this better than Swoole\Runtime::enableCoroutine();? I'm actually more confused why you're calling this with call_user_func()
There was a problem hiding this comment.
LSP-intelephense shows me an error because Swoole classes aren't recognized
There was a problem hiding this comment.
I'd rather go for end user readability than making an LSP error go away. This just raises questions as to why it's call_user_func() like is there some hidden functionality with it that the user isn't aware of.
| */ | ||
| public function __construct(protected $app) { | ||
| $this->DocsLogic = new DocsLogic($app); | ||
| final readonly class DocsController |
There was a problem hiding this comment.
The thing I've noticed with adding final is that it can make unit testing that harder if you have to mock certain parts of it. Not saying that's the case with this one, but it could be if unit testing is your goal.
There was a problem hiding this comment.
Estrategias para Escribir Código Limpio
Además de los principios fundamentales, existen una serie de estrategias prácticas que pueden ayudarte a escribir código limpio y eficiente:
KISS (Keep It Simple, Stupid): El código debe ser lo más simple posible. Evite la complejidad innecesaria y busque soluciones elegantes y eficientes.
There was a problem hiding this comment.
I understand what you're saying but I don't understand how it relates with my comment. You should of course keep your code simple and easy to be understood, I just don't see how two keywords help with that when the design around the internal code within the class is what will matter on simplicity.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 74 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
app/config/routes.php:1
- This echo statement appears to be debug code that was accidentally left in. It will output the request URL to the response body in the 404 handler, which is likely unintended behavior.
<?php
2 participants
No description provided.