chore(twig): leverage twig-cs-fixer
#60761
Conversation
There was a problem hiding this comment.
this should have a GitHub Actions workflow running the tool in CI, otherwise it won't be enforced.
regarding your summary in the description, this is not a new feature. It is about tooling for contributors and CI, but it does not bring any new feature in Symfony (and does not require a mention in changelog either, as users of Symfony won't care)
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Router/panel.html.twig
Show resolved
Hide resolved
totally agree,
top make sens, lets not add it and update the desc |
src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig
Outdated
Show resolved
Hide resolved
ff2c8ef to
7a755a8
Compare
|
i do think I've addressed the comments (standard to chose, exclude fixtures, and composer min) for the adding ci, I am not so aware of best practice of it (when no vendor lock) so please review appreciated |
f03c7a8 to
5559cb7
Compare
twig-cs-fixertwig-cs-fixer
…nglet) This PR was merged into the 3.x branch. Discussion ---------- Coding standard suggestion about empty content Hi `@fabpot` As suggested by `@stof` symfony/symfony#60761 (comment) This would allow writing `{#--#}` rather than `{#- -#}` when following the twig coding standard. Commits ------- 0b93a1f Stof suggestion about empty content
|
PR updated to use latest version |
src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig
Outdated
Show resolved
Hide resolved
|
status: needs review |
| @@ -39,7 +39,7 @@ | |||
| </div> | |||
| {% else %} | |||
| <div class="sf-reset"> | |||
| {{ render(controller('web_profiler.controller.exception_panel::body', { token: token })) }} | |||
| {{ render(controller('web_profiler.controller.exception_panel::body', {token: token})) }} | |||
There was a problem hiding this comment.
Question about the CompactHashRule rule which can transform into
| {{ render(controller('web_profiler.controller.exception_panel::body', {token: token})) }} | |
| {{ render(controller('web_profiler.controller.exception_panel::body', {token})) }} |
@GromNaN had added the "compact" syntax in Twig (same as destructuring in JS) in twigphp/Twig#327, which I find more readable and less verbose imo, inspired by what is done in JS.
We use it on all our Twig templates.
The choice of default value has been exchanged in https://github.com/VincentLanglet/Twig-CS-Fixer/pull/379/files#r2154315308 but seems quite personal. Ideally we'd like to follow the standard followed by Symfony too. What are your thoughts on this?
Thanks
/cc @VincentLanglet
There was a problem hiding this comment.
On my side I have no +-
But to be honest I rarerly see twig files using this (at least now)
There was a problem hiding this comment.
@yoriiis knowing the projects you work on, I think it makes sense to enforce the compact hash format for you. There are a lot of places where you pass variables without transformation.
It seems normal to me to have differences in coding style between projects depending on their requirements.
Here, not using this feature makes the code more expressive.
There was a problem hiding this comment.
I agree that we should not using the compact form in Symfony core.
There was a problem hiding this comment.
Should I add "Do not use compact form" to the Symfony ruleset of twig-cs-fixer then ? @fabpot
There was a problem hiding this comment.
If the ruleset is only for Symfony core, yes. For end-user apps, having it on makes sense.
There was a problem hiding this comment.
If the ruleset is only for Symfony core
Not really, I think it's kinda the "Symfony best practice/recommandations".
Currently the Twig standard is based on:
And the Symfony standard includes the Twig one and is based on
- https://symfony.com/doc/current/templates.html#template-naming
- https://symfony.com/doc/current/best_practices.html#templates
- https://symfony.com/bundles/ux-twig-component/current/index.html
- https://symfony.com/bundles/ux-turbo/current/index.html#broadcast-doctrine-entities-update
It's kinda the same idea than the Symfony Php-cs-fixer standard
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/doc/ruleSets/Symfony.rst
This PR adds config file for the tool
twig-cs-fixer(via)As per the original issue, this tool is, in my opinion, relevant here to standardize symfony source Twig file,
It follows Twig recommandation, and it's since the issue creation mentioned in the official Twig doc.
How to use:
twig-cs-fixervia https://github.com/VincentLanglet/Twig-CS-Fixer (I've usedv3.8.1now){twig-cs-fixer} --config=.twig-cs-fixer.dist.php lint --fixTODOs:
Friendly ping @VincentLanglet @smnandre as issue commenters