Use exception for system errors #3739
Conversation
There was a problem hiding this comment.
Introducing this chain file processor is necessary because the processFile() function became similar in both ApplicationFileProcessor and WorkerRunner, to make PHPStan happy, also because of complexity compliance
| /** | ||
| * @return array{system_errors: SystemError[], file_diffs: FileDiff[]} | ||
| */ |
There was a problem hiding this comment.
removed because in the interface already
| /** | ||
| * @return string[] | ||
| */ |
There was a problem hiding this comment.
removed because in the interface already
| /** | ||
| * @return array{system_errors: SystemError[], file_diffs: FileDiff[]} | ||
| */ |
There was a problem hiding this comment.
removed because in the interface already
| /** | ||
| * @return string[] | ||
| */ |
There was a problem hiding this comment.
removed because in the interface already
yguedidi
force-pushed
the
use-exception-for-system-errors
branch
from
195e1e3
to
599c0e6
Compare
| $this->invalidateFile($file); | ||
| } elseif (! $configuration->isDryRun()) { | ||
| if ($fileDiff instanceof FileDiff) { | ||
| array_unshift($fileDiffs, $fileDiff); |
There was a problem hiding this comment.
using unshift here to have current expected e2e test output green.
I'd suggest to move to fileDiffs[] = fileDiff; and adapting the expected output files
| $fileDiff = $this->chainFileProcessor->process($file, $configuration); | ||
|
|
||
| if ($fileDiff instanceof FileDiff) { | ||
| array_unshift($systemErrorsAndFileDiffs[Bridge::FILE_DIFFS], $fileDiff); |
There was a problem hiding this comment.
|
|
||
| final class ParsingException extends Exception | ||
| { | ||
| public function __construct(public readonly SystemError $systemError) |
There was a problem hiding this comment.
I'll make this property private in next rebase+push after dependency PRs are merged.
It's a leftover from a previous test
|
@samsonasik this should be ready for a first look if you want! :) |
yguedidi
force-pushed
the
use-exception-for-system-errors
branch
from
599c0e6
to
bb3e24c
Compare
|
This PR is changing too much of architecture we keep up with PHPStand and ECS. It makes it easier to maintain and extend along with other projects :) That's why I want to keep those untouched. Thanks for understanding |
|
@TomasVotruba to be honest I'm trying to understand but not really 😅 I can understand the need for synchronicity with your other projects, but I can open PRs there to align to the better architecture. What this PR is doing is not complex, it simplify the API of FileProcessorInterface from /**
* @return array{system_errors: SystemError[], file_diffs: FileDiff[]}
*/
public function process(File $file, Configuration $configuration): array;to /**
* @throws ParsingException
*/
public function process(File $file, Configuration $configuration): ?FileDiff;To me it's not a file processor responsibility to manage the list of system errors and all file diffs. BTW, this will also be helpful for the proposed idea in rectorphp/rector#7915 (if you can have a look it would be great!) |
|
@yguedidi that will break rector extension projects that consume the interface. |
I believe this makes the API for some services cleaner, and it makes sense to use exceptions for errors and catch them at the right high level, not too soon
Replace Bridge::FILES by Content::FILES #3740