Skip to content
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

refactor: refactor to templated trait+interface #7988

Merged
merged 9 commits into from
Jun 16, 2024

Conversation

keradus
Copy link
Member

@keradus keradus commented May 6, 2024

first commit is selling idea
(later commits are mess to apply the idea - very draft, half regexped)

I want to be able to benefit from phpstan analysis for configuration. now it's simply array<string, mixed>.

externalised:

@coveralls
Copy link

coveralls commented May 6, 2024

Coverage Status

coverage: 96.219% (+0.1%) from 96.121%
when pulling 38ed71a on keradus:options_template
into c3af946 on PHP-CS-Fixer:master.

@Wirone
Copy link
Member

Wirone commented May 7, 2024

I agree this is something really needed, I already approached it at some point. If this helps PHPStan and IDE to understand available configuration options, adds autosuggestion support and makes weird workarounds just to satisfy SA obsolete, then it's great, BUT only if we have fixer/script for regenerating _Configuration meta-type for each configurable fixer. I can't imagine defining/maintaining config options in 2 places manually. Also, what about config options that have a lot of acceptable values? Do we want to list them all, or use generic stuff like list<string>?

But I have a doubt - why trait was introduced? Do you want to take configuration part from AbstractFixer, because not every fixer is configurable, so the trait can be used only where ConfigurableFixerInterface is implemented? If yes, why not AbstractConfigurableFixer extends AbstractFixer? As I understand, we don't need to think about backward compatibility here since AbstractFixer is marked as @internal?

In general, this is rather achievable without the trait part:

  • add @template TConfig of array<string, mixed> to AbstractFixer (along with template on ConfigurableFixerInterface?)
  • change AbstractFixer::$configuration's phpDoc to @var null|TConfig and align related types accordingly
  • each configurable fixer then need to do @extends AbstractFixer<{actual: 'shape'|'with'|'options'}>

But yeah, it does look weird when you need to define @extends and provide value for TConfig template even for fixers that are not configurable. I am wondering if it's possible to fallback to default value in such scenario 🤔.

@keradus
Copy link
Member Author

keradus commented May 13, 2024

I agree this is something really needed, phpstan/phpstan#9686 at some point.

indeed, that was my inspiration. [and I do not believe we need custom extension]

UT only if we have fixer/script for regenerating _Configuration meta-type

I like the idea. will see what i can craft

Also, what about config options that have a lot of acceptable values? Do we want to list them all, or use generic stuff like list?

likely sth generic only. better than nothing 🤷🏻
we have places when we allow subset of ['aaa', 'bbb', 'ccc', 'ddd', ...], or more complex structures. I would love to avoid overdoing it.

But I have a doubt - why trait was introduced?

making AbstractFixer a template class was complaining to me for non-configurable fixer to not specify template type, and making non-configurable fixer to declare extends AbstractFixer<void> was weird to me

*
* @phpstan-type _AutogeneratedInputConfiguration array{
* comment_type?: 'PHPDoc'|'comment',
* header: string,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only mandatory config key in whole project

@keradus keradus force-pushed the options_template branch 3 times, most recently from 4fd6d85 to 2a5d59e Compare June 16, 2024 11:39
@keradus keradus marked this pull request as ready for review June 16, 2024 11:40
@keradus keradus force-pushed the options_template branch 3 times, most recently from d14aa8e to 7dcc498 Compare June 16, 2024 13:27
@keradus keradus force-pushed the options_template branch 2 times, most recently from 57d1002 to 1079491 Compare June 16, 2024 13:40
@jorismak
Copy link

This breaks certain custom-rules packages that work fine with 3.59.2. Like https://github.com/PedroTroller/PhpCSFixer-Custom-Fixers.

Is that a bug in the custom-rules package, or is this code more BC breaking than expected?

@keradus
Copy link
Member Author

keradus commented Jul 12, 2024

@jorismak ,
I discourage to use any of the INTERNAL class from Fixer's core repo in own repos, as they are NOT with BC promise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants