-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
chore(twig): leverage twig-cs-fixer
#60761
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
base: 7.4
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
<div{% with {attr: row_attr|merge({class: (row_attr.class|default('') ~ ' form-group')|trim})} %}{{ block('attributes') }}{% endwith %}>{#--#} | ||
<div class="{{ block('form_label_class') }}"></div>{#--#} | ||
<div{% with {attr: row_attr|merge({class: (row_attr.class|default('') ~ ' form-group')|trim})} %}{{ block('attributes') }}{% endwith %}>{#- -#} | ||
<div class="{{ block('form_label_class') }}"></div>{#- -#} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VincentLanglet I think even your opinionated standard should allow the short whitespace-control command {#--#}
without requiring a space.
Requiring a space around the comment text is good (and is part of the official Twig coding standards), but is not worth it if there is no text IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's better.
For this rule I blindly follow the twig coding standard coming from https://twig.symfony.com/doc/3.x/coding_standards.html#coding-standards
Put exactly one space after the start of a delimiter ({{, {%, and {#) and before the end of a delimiter (}}, %}, and #}):
When using the whitespace control character, do not put any spaces between it and the delimiter:
So, what might be great would be to update this page and then to update the rule on my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened the PR on the standard here twigphp/Twig#4649
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 |
e2f08ac
to
f03c7a8
Compare
twig-cs-fixer
twig-cs-fixer
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-fixer
via https://github.com/VincentLanglet/Twig-CS-Fixer (I've usedv3.7.1
now){twig-cs-fixer} --config=.twig-cs-fixer.dist.php lint --fix
TODOs:
Friendly ping @VincentLanglet @smnandre as issue commenters