@TomasVotruba @samsonasik any feedback on the approach here?

This configs seems to expose value as changeable on the fly. That is active record pattern that could be hard to track.

Instead, the config must be write-only as much as possible, so we can relly on it 100 %.

If I understand correctly, there are 3 changes at once. Better do one change per PR, so they're easier to review and accept/decline, and avoid staling like this one.

If I understand correctly, there are 3 changes at once. Better do one change per PR, so they're easier to review and accept/decline, and avoid staling like this one.

To me there are two, the renaming (I will extract the commit in a dedicated PR) and the use of a ParallelConfig to configure parallel related options.
What is the third one you see?

This configs seems to expose value as changeable on the fly. That is active record pattern that could be hard to track.

I don't understand this feedback, could you elaborate please?

I don't understand this feedback, could you elaborate please?

Sure, this line exactly https://github.com/rectorphp/rector-src/pull/3602/files#diff-7670175f1f1c668d4a1611d0e211d616b84d433c7b8280f88900fe1445e5b792R53

Introduces fluent/ambiguous API that suggest this value should be changed on the fly.

Why is this bad? I'd argue with reasons mentioned in https://ocramius.github.io/blog/fluent-interfaces-are-evil/ :)

@TomasVotruba to be honest I would have liked to remove the parameter to just have parallel() and the fluent interface for configuring parallel settings.

Introduces fluent/ambiguous API that suggest this value should be changed on the fly.

Why is this bad? I'd argue with reasons mentioned in https://ocramius.github.io/blog/fluent-interfaces-are-evil/ :)

I totally agree! I'm not a fan of fluent interfaces neither for all the mentioned reasons :)
But I think here it's a valid use case, it's configuration.
I see it like the TreeBuilder of Symfony Config component

Also, here issues with fluent interface don't apply I think:

• Fluent Interfaces break Encapsulation: it's final
• Fluent Interfaces break Decorators (and sometimes Composition): it's final
• Fluent Interfaces are harder to Mock: a POPO (Plain Old PHP Object)
• Fluent Interfaces make diffs harder to read: up to end user writing the config
• Fluent Interfaces cause BC breaks during early development stages: not early development

To me this looks really clear from an end user perspective:

$rectorConfig->parallel()->jobSize(50)->jobTimeout(300);

I can understand you don't have same opinion as the maintainer, maybe get more feedback from end users?

Here the alternatives I can think of:

• more parameters to parallel() method, list may become long, what if you want to configure just a few? could be mitigated using named arguments
• new methods directly on RectorConfig, next to the existing ->parallel(), similar to the actual ->disableParallel()
• make the config closure takes a ParallelConfig $parallelConffig second parameter along the actual RectorConfig $rectorConfig, to be used directly in the closure. This will allow to get rid of the fluent interface seems not possible as rector config is a normal Symfony DI file

@TomasVotruba forgot to mention that I kept the parameters of ->parallel() for backward compatibility :)