Like what Core is planning to do: #3284817: Adopt vincentlanglet/twig-cs-fixer for Twig coding standards

Our templates are already 99% compliant, the main nicety is the removal of quote in mapping keys:

-      {{ 'Replaces'|t }}: <a href="{{ url('ui_patterns_library.single', {'provider': component.replaces|split(':') |first, 'machineName': component.replaces|split(':')|last }) }}">{{ component.replaces }}</a>
+      {{ 'Replaces'|t }}: <a href="{{ url('ui_patterns_library.single', {provider: component.replaces|split(':')|first, machineName: component.replaces|split(':')|last}) }}">{{ component.replaces }}</a>

We need to do 2 tasks:

  • run the twig linter on existign codebase
  • add the linter to our CI pipeline

Issue fork ui_patterns-3483831

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

  • pdureau committed 427b8366 on 2.0.x 
    Issue #3483831 by pdureau: apply vincentlanglet/twig-cs-fixer to...
pdureau’s picture

pdureau commented
Title: [2.0.0-beta5] Adopt vincentlanglet/twig-cs-fixer for Twig coding standards » [2.0.0-rc] Adopt vincentlanglet/twig-cs-fixer for Twig coding standards

run the twig linter on existign codebase

Done in beta5: https://git.drupalcode.org/project/ui_patterns/-/commit/427b83660dffb549...

add the linter to our CI pipeline

Moved to RC scope, and it will be the opportunity to run the command again.

smovs’s picture

smovs commented
Assigned: Unassigned » smovs
pdureau’s picture

pdureau commented
Title: [2.0.0-rc] Adopt vincentlanglet/twig-cs-fixer for Twig coding standards » [2.0.0-rc1] Adopt vincentlanglet/twig-cs-fixer for Twig coding standards

smovs opened merge request !298

smovs’s picture

smovs commented
Assigned: smovs » Unassigned
Status: Active » Needs review

Hi guys!
I added the twig linter to the CI pipeline.

Please review.

pdureau’s picture

pdureau commented
Assigned: Unassigned » christian.wiedemann

Hi smos,

Christian will review your gitlab CI addition.

It seems you didn't run the linter on the existing UIP2 codebase, because I have this:

+++ b/tests/modules/ui_patterns_test/components/test-component/test-component.twig
@@ -1,5 +1,5 @@
 {% set variant = variant|default('') %}
-<div{{ attributes.addClass(['ui-patterns-test-component', 'ui-patterns-test-component-variant-' ~ variant ]) }}>
+<div{{ attributes.addClass(['ui-patterns-test-component', 'ui-patterns-test-component-variant-' ~ variant]) }}>
   <div class="ui-patterns-props-string">
     {{ string }}
   </div>
smovs’s picture

smovs commented

Hi pdureau,
Thank you for your feedback.
Indeed, I didn't run the linter when finished with GitLab CI. I will check it.

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented

one comment about the "with_context=false".
I knew the linter wanted to put "with_context: false", but it is written everywhere that the right code portion is "with_context=false".
so i was like confused.
what do you guys think?

smovs’s picture

smovs commented

I was also a bit confused and double-checked it on the Twig CS page. They suggest use: instead of =
Anyway, we can use our custom ruleset to override if necessary.

Twig CS here: Link to CS

christian.wiedemann’s picture

christian.wiedemann commented

I checked it right now, and it is fine for me. We use "only" . (Is it the same)

{% embed "" {param: param} only %}
{% endembed %}
pdureau’s picture

pdureau commented
Assigned: christian.wiedemann » Unassigned
Status: Needs review » Reviewed & tested by the community

Anyway, we can use our custom ruleset to override if necessary.

No custom ruleset. Let's just follow default vincentlanglet/twig-cs-fixer without overthinking

  • pdureau committed f0735579 on 2.0.x authored by smovs 
    Issue #3483831 by smovs, pdureau: Adopt vincentlanglet/twig-cs-fixer for...
pdureau’s picture

pdureau commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.