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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

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.

phpunit.xml.dist Outdated
@@ -55,6 +55,6 @@
<php>
<ini name="zend.enable_gc" value="0"/>
<ini name="memory_limit" value="10G"/>
<env name="FAST_LINT_TEST_CASES" value="0"/>
<env name="FAST_LINT_TEST_CASES" value="1"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

to be reverted

@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

# Conflicts:
#	src/Fixer/Alias/RandomApiMigrationFixer.php
@keradus keradus force-pushed the options_template branch 2 times, most recently from 7257101 to d30b1d3 Compare May 23, 2024 08:17
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.

None yet

3 participants