-
Notifications
You must be signed in to change notification settings - Fork 0
Unit tests added #1
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
Conversation
lloc
commented
Oct 22, 2025
- Unit Tests added
- Some refactoring to get PHPStan level raised to 10
Summary of ChangesHello @lloc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's code quality and reliability by integrating a suite of new unit tests for various commands. Concurrently, it elevates the static analysis standard to PHPStan level 10, ensuring more robust and type-safe code. These changes collectively aim to improve maintainability and reduce potential bugs by catching issues earlier in the development cycle. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces unit tests and refactors the code to improve PHPStan level. The changes include adding test files for several commands, modifying .gitignore and phpunit.xml.dist, and adjusting code in CreatePoCommand.php, CustomCommand.php, and ShellRunner.php. The refactoring primarily focuses on type hinting and nullability to enhance code quality and static analysis.
| * @param string[] $output The output in an array. | ||
| * @param int $result_code The result code of the command execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the type hints from nullable to non-nullable is a good step towards stricter type checking. However, the exec function relies on the exec PHP function, which might not always populate the $output and $result_code variables, especially in error scenarios. If the underlying exec function fails, these variables might remain uninitialized, leading to unexpected behavior in the calling code.
Consider adding error handling to check if $output and $result_code are properly populated by the exec call and throw an exception or return an error code if they are not.
| $output_lines = []; | ||
| $exit_code = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing $output_lines and $exit_code before calling $this->runner->exec is good for ensuring they have default values. However, these variables are passed by reference to the exec method. If exec fails to populate these variables, the code might proceed with potentially incorrect or uninitialized data. It would be safer to check if these variables are populated after the exec call and handle the error if they are not.
Consider adding a check after the exec call to ensure $output_lines and $exit_code are properly populated.
| $locale = $input->getArgument( 'locale' ); | ||
| if ( empty( $locale ) || ! is_string( $locale ) ) { | ||
| $locale = self::DEFAULT_LANGUAGE; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition empty( $locale ) || ! is_string( $locale ) can be simplified by using ! is_string( $locale ) alone. empty already checks for null, '', false, 0, 0.0, '0', [], so the additional empty check is redundant and makes the code less readable.
Consider removing the empty check.
if ( ! is_string( $locale ) ) {
$locale = self::DEFAULT_LANGUAGE;
}| $command = new MakePotCommand($runner); | ||
|
|
||
| $command = $application->find( 'i18n:make-pot' ); | ||
| $tester = new CommandTester( $command ); | ||
| $this->application = new Application(); | ||
| $this->application->setAutoExit(false); // Important for tests | ||
| $this->application->add($command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $command variable is being reassigned with a new instance of MakePotCommand inside the setUp method. This is unnecessary because the $command variable is only used within the setUp method and doesn't need to be stored as a class property. This reassignment can be removed to simplify the code and reduce memory usage.
Consider removing the reassignment of the $command variable in the setUp method.
$runner = $this->createMock(ShellRunner::class);
$runner->method('exec')
->willReturnCallback(function ($cmd, &$output, &$exitCode) {
$output = ['Mocked .pot generation'];
$exitCode = 0;
});
$this->application = new Application();
$this->application->setAutoExit(false); // Important for tests
$this->application->add(new MakePotCommand($runner));