refact(core): Refactor FriendlyErrorFormatter for improved structure … #11
Conversation
WalkthroughThe pull request introduces multiple changes across various files, including the addition of a new job in the GitHub Actions workflow for checking typos, updates to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
yamadashy
force-pushed
the
refact/formatter
branch
from
c4dce70
to
465fc2d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (7)
src/Config/FriendlyFormatterConfig.php (1)
1-25: Overall, good introduction of the configuration class with room for improvement.The
FriendlyFormatterConfigclass is a good addition for managing configuration settings. To further improve it:
- Implement getter methods for the private properties to address the "property only written" warnings.
- Add input validation in the constructor to ensure robust handling of configuration parameters.
- Consider adding PHPDoc comments to the class and constructor to provide clear documentation of the class's purpose and parameter requirements.
These enhancements will improve the usability and maintainability of the configuration class.
Tools
PHPStan
8-8: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$lineBefore is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
11-11: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$lineAfter is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
14-14: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$editorUrl is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
src/CodeHighlight/FallbackHighlighter.php (1)
Line range hint
11-33: Consider improving documentation and error handling.While the implementation of
getCodeSnippetlooks correct, there are two areas where it could be improved:
Documentation: The color formatting tags (e.g.,
<fg=gray>,<fg=red>) are not standard PHP. It would be helpful to add a comment explaining which console output library these tags are intended for.Error Handling: Consider adding explicit error handling for edge cases such as empty file content or out-of-bounds line numbers.
Here's a suggested improvement for error handling:
public function getCodeSnippet(string $fileContent, int $lineNumber, int $lineBefore, int $lineAfter): string { + if (empty($fileContent)) { + return 'Empty file content'; + } + $lines = explode("\n", $fileContent); $totalLines = \count($lines); + if ($lineNumber < 1 || $lineNumber > $totalLines) { + return sprintf('Line number %d is out of bounds (1-%d)', $lineNumber, $totalLines); + } + $startLine = max(1, $lineNumber - $lineBefore); $endLine = min($totalLines, $lineNumber + $lineAfter); // ... rest of the method }Also, consider adding a comment at the top of the class to explain the color formatting:
/** * FallbackHighlighter provides a fallback method for highlighting code snippets. * * Note: This class uses color formatting tags (e.g., <fg=gray>) which are * specific to the Symfony Console component. These tags will be interpreted * by the Symfony Console output formatter. */ class FallbackHighlightersrc/ErrorFormat/SummaryWriter.php (2)
8-9: Consider making the class final.If this class is not intended to be extended, it's a good practice to declare it as
final. This clearly communicates the design intent and can help with performance optimizations.Consider updating the class declaration:
-class SummaryWriter +final class SummaryWriter
1-32: Consider enhancing class design for better flexibility and testability.While the class adheres to the Single Responsibility Principle, there are a couple of suggestions to further improve its design:
Dependency Injection: Instead of creating the
$errorCounterarray internally, consider injecting it or its factory. This would make the class more flexible and easier to test.Interface Segregation: The
Outputinterface might be too broad. Consider creating a more specific interface (e.g.,SummaryOutput) with only the methods needed for this class.Here's a sketch of how these improvements might look:
interface ErrorCounter { public function countErrors(AnalysisResult $analysisResult): array; } interface SummaryOutput { public function writeLineFormatted(string $line): void; } final class SummaryWriter { public function __construct( private ErrorCounter $errorCounter, private SummaryOutput $output ) {} public function writeGroupedErrorsSummary(AnalysisResult $analysisResult): void { $errorCounts = $this->errorCounter->countErrors($analysisResult); arsort($errorCounts); $this->output->writeLineFormatted('📊 Error Identifier Summary:'); $this->output->writeLineFormatted('────────────────────────────'); foreach ($errorCounts as $identifier => $count) { $this->output->writeLineFormatted(sprintf(' %d %s', $count, $identifier)); } } }This design would make the
SummaryWritermore flexible, easier to test, and more aligned with SOLID principles..github/workflows/tests.yml (1)
54-63: Consider making the typo check a required step for PRs.The new typo checking job is a valuable addition to the workflow. To ensure its effectiveness, consider making it a required check before merging pull requests.
You can achieve this by:
- Adding a status check in your repository settings to require the "Check for typos" job to pass before merging.
- Optionally, you could add a
needsclause to theunit_testsjob to ensure typo checking is done before running tests:unit_tests: needs: typos # ... rest of the unit_tests job configurationThis would ensure that typos are caught early in the development process, maintaining code quality.
tests/CodeHighlighterTest.php (1)
Line range hint
11-13: Update@coversDefaultClassannotationThe
@coversDefaultClassannotation still refers to the old namespace. It should be updated to reflect the new namespace of theCodeHighlighterclass.Please update the annotation as follows:
/** * @internal * - * @coversDefaultClass \Yamadashy\PhpStanFriendlyFormatter\CodeHighlighter + * @coversDefaultClass \Yamadashy\PhpStanFriendlyFormatter\CodeHighlight\CodeHighlighter */src/ErrorFormat/ErrorWriter.php (1)
100-109: Consider a small optimization ingetFormattedTipmethodThe
getFormattedTipmethod correctly formats tips associated with errors. However, a small optimization can be made to simplify the code:Consider replacing the current implementation with:
private function getFormattedTip(Error $error): ?string { return $error->getTip() !== null ? str_replace("\n", "\n ", $error->getTip()) : null; }This change eliminates the need for the intermediate
$tipvariable and the explicitnullcheck, making the code more concise.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .github/workflows/tests.yml (1 hunks)
- .gitignore (1 hunks)
- src/CodeHighlight/CodeHighlighter.php (1 hunks)
- src/CodeHighlight/FallbackHighlighter.php (1 hunks)
- src/Config/FriendlyFormatterConfig.php (1 hunks)
- src/ErrorFormat/ErrorWriter.php (1 hunks)
- src/ErrorFormat/SummaryWriter.php (1 hunks)
- src/FriendlyErrorFormatter.php (2 hunks)
- tests/CodeHighlighterTest.php (1 hunks)
Additional context used
PHPStan
src/Config/FriendlyFormatterConfig.php
8-8: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$lineBefore is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
11-11: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$lineAfter is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
14-14: Property Yamadashy\PhpStanFriendlyFormatter\Config\FriendlyFormatterConfig::$editorUrl is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
src/ErrorFormat/ErrorWriter.php
18-18: Property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$config is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
61-61: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$lineAfter.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
61-61: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$lineBefore.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
74-74: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$editorUrl.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
Additional comments not posted (16)
.gitignore (1)
5-5: LGTM: Appropriate addition to .gitignoreThe addition of
.phpunit.cacheto the .gitignore file is a good practice. It's consistent with ignoring other cache files already present in the list, such as.phpunit.result.cache. This change will help keep the repository clean by preventing PHPUnit cache files from being tracked in version control.src/Config/FriendlyFormatterConfig.php (1)
1-6: LGTM: Namespace and class definition are well-structured.The use of strict types, proper namespace declaration, and matching class name to file name adhere to PHP best practices.
src/ErrorFormat/SummaryWriter.php (1)
1-7: LGTM: File structure and namespace declaration are correct.The file follows PSR-4 autoloading standard and PHP best practices. The strict types declaration, namespace, and imports are properly set up.
src/ErrorFormat/ErrorWriter.php (4)
1-26: LGTM: Well-structured class and constructorThe
ErrorWriterclass is well-organized, following PSR-12 coding standards. The use of dependency injection in the constructor is a good practice, promoting loose coupling and easier testing.Tools
PHPStan
18-18: Property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$config is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
84-90: LGTM:writeNotFileSpecificErrorsmethodThe
writeNotFileSpecificErrorsmethod is concise and correctly handles non-file-specific errors. It formats and outputs each error message as expected.
92-98: LGTM:writeWarningsmethodThe
writeWarningsmethod effectively formats and outputs warnings from the analysis result. The use of a yellow warning symbol (⚠) helps differentiate warnings from errors, improving readability.
1-110: Summary: Well-structured class with minor improvements neededThe
ErrorWriterclass is generally well-implemented and follows good coding practices. Here's a summary of the main points:
- The class structure and method implementations are solid.
- There are undefined properties (
$lineBefore,$lineAfter,$editorUrl) that need to be properly defined and initialized.- The
getFormattedTipmethod could benefit from a small optimization.- The
$configproperty is currently unused and should be addressed.Addressing these points will improve the overall quality and maintainability of the code.
Tools
PHPStan
18-18: Property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$config is never read, only written.
See: https://phpstan.org/developing-extensions/always-read-written-properties(property.onlyWritten)
61-61: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$lineAfter.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
61-61: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$lineBefore.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
74-74: Access to an undefined property Yamadashy\PhpStanFriendlyFormatter\ErrorFormat\ErrorWriter::$editorUrl.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property(property.notFound)
src/FriendlyErrorFormatter.php (9)
10-12: Imports new classes for enhanced modularity.The addition of
FriendlyFormatterConfig,ErrorWriter, andSummaryWriterimports improves code organization and promotes separation of concerns.
19-20: Introduces a configuration property for centralized settings.Adding the
$configproperty withFriendlyFormatterConfigcentralizes the formatter's configuration, enhancing maintainability.
25-29: Initializes configuration within the constructor.Encapsulating the configuration parameters into
FriendlyFormatterConfigsimplifies the constructor and improves code clarity.
38-38: Refactors no-error handling into a separate method.Moving the no-error logic to
handleNoErrorsimproves readability and adheres to the Single Responsibility Principle.
43-46: Delegates error writing to theErrorWriterclass.Instantiating
ErrorWriterand offloading error output responsibilities enhances modularity and code organization.
48-49: UtilizesSummaryWriterfor error summarization.Employing
SummaryWriterto handle grouped error summaries furthers separation of concerns and readability.
51-51: Extracts result message logic intowriteResultMessagemethod.Refactoring the result message output improves maintainability and keeps
formatErrorsfocused on high-level flow.
56-60: DefineshandleNoErrorsmethod for cleaner error handling.Encapsulating the success message in
handleNoErrorsenhances code clarity and reduces duplication.
Line range hint
63-75: Verifying message styling when warnings exist without errors.When there are warnings but no errors, the final message is displayed using the
warningstyle. Please verify if this aligns with the intended user experience or if a different style (e.g.,info) would be more appropriate to avoid confusion.
yamadashy
force-pushed
the
refact/formatter
branch
3 times, most recently
from
f404d76
to
933ef9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .github/workflows/tests.yml (1 hunks)
- .gitignore (1 hunks)
- .typos.toml (1 hunks)
- src/CodeHighlight/CodeHighlighter.php (3 hunks)
- src/CodeHighlight/FallbackHighlighter.php (1 hunks)
- src/Config/FriendlyFormatterConfig.php (1 hunks)
- src/ErrorFormat/ErrorWriter.php (1 hunks)
- src/ErrorFormat/SummaryWriter.php (1 hunks)
- src/FriendlyErrorFormatter.php (2 hunks)
- tests/CodeHighlighterTest.php (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- .github/workflows/tests.yml
- .gitignore
- src/CodeHighlight/FallbackHighlighter.php
- src/Config/FriendlyFormatterConfig.php
- src/ErrorFormat/ErrorWriter.php
- src/ErrorFormat/SummaryWriter.php
- src/FriendlyErrorFormatter.php
- tests/CodeHighlighterTest.php
Additional comments not posted (5)
.typos.toml (2)
1-4: LGTM: Appropriate exclusion of .git directoryThe configuration to exclude the ".git/" directory from typo checking is a good practice. This directory contains version control information and is not relevant for typo checks, which helps to improve the efficiency of the typo-checking process.
5-5: Consider the implications of including hidden files in typo checksSetting
ignore-hidden = falsewill include hidden files in the typo-checking process. While this can be beneficial for catching typos in configuration files (which are often hidden), it may also increase the number of false positives and the overall processing time.To assess the impact of this setting, you may want to run the following command to see how many hidden files exist in your project:
Consider whether checking these files for typos is necessary for your project. If you decide to keep this setting, make sure to monitor the typo-checking results and adjust as needed.
Verification successful
Including hidden files in typo checks is acceptable.
After counting, there are only 13 hidden files in the project. Including them in typo checks is manageable and beneficial for catching potential typos in configuration files without significantly impacting processing time or increasing false positives.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Count the number of hidden files in the project find . -type f -name ".*" | wc -lLength of output: 38
src/CodeHighlight/CodeHighlighter.php (3)
3-3: Approved: Namespace updated to reflect new structureThe namespace change to
Yamadashy\PhpStanFriendlyFormatter\CodeHighlightaligns the code with the directory structure and adheres to PSR-4 autoloading standards, improving code organization.
5-6: Approved: Added imports for legacy compatibilityThe addition of
LegacyConsoleColorandLegacyHighlighterimports ensures compatibility with older versions of the dependencies, enhancing the flexibility and robustness of the code.
33-34: Approved: Correct instantiation of legacy classesThe initialization of
LegacyConsoleColorandLegacyHighlighterwhen the legacy classes are available is appropriate and ensures backward compatibility with older versions.
yamadashy
force-pushed
the
refact/formatter
branch
from
933ef9d
to
f419553
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/FriendlyErrorFormatter.php (1)
Line range hint
63-77: LGTM: New writeResultMessage method enhances code structureThe new
writeResultMessagemethod effectively separates the logic for creating and displaying the final result message. This change improves code organization and readability. The method handles both error and warning counts appropriately and applies the correct output styling.Consider using string interpolation for better readability:
$finalMessage = "{$analysisResult->getTotalErrorsCount()} error" . ($analysisResult->getTotalErrorsCount() !== 1 ? 's' : ''); if ($warningsCount > 0) { $finalMessage .= " and {$warningsCount} warning" . ($warningsCount !== 1 ? 's' : ''); } $finalMessage = "Found {$finalMessage}";This change would make the string construction more concise and easier to read.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .github/workflows/tests.yml (1 hunks)
- .gitignore (1 hunks)
- .typos.toml (1 hunks)
- src/CodeHighlight/CodeHighlighter.php (3 hunks)
- src/CodeHighlight/FallbackHighlighter.php (1 hunks)
- src/Config/FriendlyFormatterConfig.php (1 hunks)
- src/ErrorFormat/ErrorWriter.php (1 hunks)
- src/ErrorFormat/SummaryWriter.php (1 hunks)
- src/FriendlyErrorFormatter.php (2 hunks)
- tests/CodeHighlighterTest.php (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- .github/workflows/tests.yml
- .gitignore
- .typos.toml
- src/CodeHighlight/CodeHighlighter.php
- src/CodeHighlight/FallbackHighlighter.php
- src/Config/FriendlyFormatterConfig.php
- src/ErrorFormat/ErrorWriter.php
- src/ErrorFormat/SummaryWriter.php
- tests/CodeHighlighterTest.php
Additional comments not posted (5)
src/FriendlyErrorFormatter.php (5)
10-12: LGTM: New imports added correctlyThe new import statements for
FriendlyFormatterConfig,ErrorWriter, andSummaryWriterare correctly added and align with the refactoring changes. They follow PSR-4 autoloading standards and are placed appropriately in the file.
19-20: LGTM: New configuration property addedThe new
$configproperty of typeFriendlyFormatterConfigis correctly added and typed. This change aligns with the refactoring to use a configuration object, improving the overall structure of the class.
38-38: LGTM: Improved modularity in formatErrors methodThe
formatErrorsmethod has been refactored to useErrorWriterandSummaryWriterobjects, which improves separation of concerns and modularity. The introduction ofhandleNoErrorsandwriteResultMessagemethods further enhances the structure and readability of the code.These changes make the method easier to understand and maintain, while preserving the overall logic flow.
Also applies to: 43-46, 48-49, 51-51, 53-53
56-60: LGTM: New handleNoErrors method improves code organizationThe new
handleNoErrorsmethod effectively separates the logic for handling cases with no errors or warnings. This change improves code organization and follows the single responsibility principle. The method is concise and focused, making the code more readable and maintainable.
Line range hint
1-78: Excellent refactoring to improve modularity and separation of concernsThe changes made to the
FriendlyErrorFormatterclass significantly improve its structure and maintainability. The introduction ofFriendlyFormatterConfig,ErrorWriter, andSummaryWriterclasses helps to separate concerns and make the code more modular. The refactoring of theformatErrorsmethod and the addition of new helper methods (handleNoErrorsandwriteResultMessage) enhance readability and organization.These changes align well with the PR objectives of improving structure and separation of concerns. The refactoring maintains backward compatibility while providing a more flexible and maintainable codebase.
Overall, this is a high-quality refactoring that should make the code easier to understand, maintain, and extend in the future.
…and separation of concerns
yamadashy
force-pushed
the
refact/formatter
branch
from
f419553
to
fe47f25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
.php-cs-fixer.dist.php (1)
9-10: LGTM! Performance optimization added.The addition of parallel configuration using
ParallelConfigFactory::detect()is a good optimization. This change can potentially improve the performance of PHP CS Fixer by enabling parallel processing.Consider documenting this change in the project's changelog or README, as it might lead to noticeable performance improvements, especially on larger codebases or in CI/CD pipelines.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- .github/workflows/tests.yml (1 hunks)
- .gitignore (1 hunks)
- .php-cs-fixer.dist.php (1 hunks)
- .typos.toml (1 hunks)
- src/CodeHighlight/CodeHighlighter.php (3 hunks)
- src/CodeHighlight/FallbackHighlighter.php (1 hunks)
- src/Config/FriendlyFormatterConfig.php (1 hunks)
- src/ErrorFormat/ErrorWriter.php (1 hunks)
- src/ErrorFormat/SummaryWriter.php (1 hunks)
- src/FriendlyErrorFormatter.php (2 hunks)
- tests/CodeHighlight/CodeHighlighterTest.php (5 hunks)
Files skipped from review as they are similar to previous changes (9)
- .github/workflows/tests.yml
- .gitignore
- .typos.toml
- src/CodeHighlight/CodeHighlighter.php
- src/CodeHighlight/FallbackHighlighter.php
- src/Config/FriendlyFormatterConfig.php
- src/ErrorFormat/ErrorWriter.php
- src/ErrorFormat/SummaryWriter.php
- src/FriendlyErrorFormatter.php
Additional comments not posted (4)
tests/CodeHighlight/CodeHighlighterTest.php (4)
3-3: LGTM: Namespace update improves organizationThe namespace change from
TeststoTests\CodeHighlightaligns well with the file's location and provides better organization for the test cases.
52-52: LGTM: Consistent file path updates throughout test casesThe remaining file path changes are consistent with the earlier modifications, maintaining uniformity across all test cases.
Also applies to: 69-69, 77-77
22-22: LGTM: File paths updated to reflect new directory structureThe file paths in the test data provider have been consistently updated to reflect the new directory structure. This change aligns with the refactoring efforts.
Let's verify the existence of these files:
#!/bin/bash # Verify the existence of test data files fd -e php AnalysisTargetFoo.php AnalysisTargetBar.phpAlso applies to: 36-36
7-7: LGTM: Updated use statement reflects refactoringThe updated use statement for CodeHighlighter aligns with the refactoring mentioned in the PR objectives. This change improves the structure and organization of the codebase.
To ensure consistency, let's verify the new namespace:
…and separation of concerns
Summary by CodeRabbit
New Features
FriendlyFormatterConfigclass for enhanced configuration management.ErrorWriterandSummaryWriterclasses to improve error reporting and summarization.Bug Fixes
.gitignoreto ignore.phpunit.cache, preventing unnecessary file tracking.Refactor
CodeHighlighterandFallbackHighlighterclasses.FriendlyErrorFormatterto streamline error handling and output management.Tests