[config] Extract RectorConfigValidator and handle config validation there #5404
Conversation
TomasVotruba
force-pushed
the
tv-validate-config
branch
from
139af02
to
8b48bf5
Compare
packages/Config/RectorConfig.php
Outdated
| @@ -81,7 +83,7 @@ public function enableCollectors(): void | |||
| SimpleParameterProvider::setParameter(Option::COLLECTORS, true); | |||
| } | |||
|
|
|||
| public function parallel(int $seconds = 120, int $maxNumberOfProcess = 16, int $jobSize = 15): void | |||
| public function parallel(int $seconds = 120, int $maxNumberOfProcess = 32, int $jobSize = 20): void | |||
There was a problem hiding this comment.
Let's stay with PHPStan default to avoid per-project upgrades of this file:
https://phpstan.org/config-reference#parallel-processing
There was a problem hiding this comment.
iirc, 15 is to make less memory, /cc @staabm ?
There was a problem hiding this comment.
These are designed be configured per project as differt by OS, machine and project. Also low memory is not the goal, usually speed is. Let's stay in sync with PHPStan to keep it simple.
There was a problem hiding this comment.
rector has very different usage pattern of memory.
these numbers were used to allow running rector on big projects without endless amounts of memory.
I don't think its a good idea to change this numbers without a motivation/goal
There was a problem hiding this comment.
Also low memory is not the goal, usually speed is.
its not about "low memory". its about sucking all RAM out of a system and running into swap, which turns the system to be super slow
There was a problem hiding this comment.
see e.g. https://github.com/complex-gmbh/daiber/pull/5531 in which github actions run into timeouts with the new config, but ran successfully with the previous defaults
There was a problem hiding this comment.
@TomasVotruba I think rollback to previous config is better, since the previous version is with @staabm experience on big project and large files on his PR with max chunk per-worker set:
also I test that 60 seconds timeout seems timeout on CodeIgniter4 project
https://github.com/codeigniter4/CodeIgniter4/actions/runs/7462946128/job/20306475124#step:9:11
set to 120 second solve it.
There was a problem hiding this comment.
These are project specific values, there is no reason to promote them to default as we do not have data about average Rector usage nor runtime. On a different project setting higher values would lead to late feedback, etc.
Customize those values to fit the particular project.
| 'The services() method is deprecated. Use $rectorConfig->singleton(ServiceType::class) instead', | ||
| E_USER_ERROR | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is no longer needed 👍
TomasVotruba
force-pushed
the
tv-validate-config
branch
3 times, most recently
from
f674af2
to
6700e1c
Compare
|
Closing to avoid further back and forth changes. Customize per project. |
No description provided.