Remove excess spacing before multiline block

[ruudk]

Fixes #245

[VincentLanglet]

If you want to take a look, I added a commit which seems to fix everything except

Error: Cannot unpack array with string keys

which is a PHP 8 specific error.

Some shared thought:
I'm not sure the AbstractSpacingRule should implements ConfigurableRuleInterface by default. We might prefer to enable the config manually on the different rules. This PR is adding skipIfNewLine option for the rules:

• OperatorSpacingRule
• DelimiterSpacingRule
• BlockNameSpacingRule
• PunctuationSpacingRule

Maybe it's better to just make the DelimiterSpacingRule configurable for now, and add

public function __construct() {
    parent::__construct();
}

to other rules in order to remove the param from them (so far) and wait for feature request (?) This way we could know

• if the option is required
• if the option should be the first param of the rule (as an example PunctuationSpacingRule already has option).

Does it seem ok to you ?

[ruudk]

@VincentLanglet Thanks so much for helping me out! I agree with what you're saying. I just rebased the PR and made the suggested changes. Let me know what you think!

[ruudk]

Applied the feedback!

[ruudk]

Pushed an updated test that now fails.. I think it should also remove the spaces.

[VincentLanglet]

I wont be able to debug this before the next week since I dont have a computer ATM.

[VincentLanglet]

Pushed an updated test that now fails.. I think it should also remove the spaces.

Hi @ruudk, I don't have any real failure with

{%

    trans with {
    contactUrl: 'http://contact',
}

    %}my.key{% endtrans %}

I just need to update tests/failure message.

But I think the failed test you wanted is the one with only one space

{%

    trans with {
    contactUrl: 'http://contact',
}

 %}my.key{% endtrans %}

Is it the issue you encounter ? If yes, I'll take a look.

[ruudk]

Yes, I couldn't provide a good failing test.

So I think what we want is that with skipIfNewLine=true (default) the following will be changed:

 {%
 
     trans with {
     contactUrl: 'http://contact',
 }
 
-    %}my.key{% endtrans %}
+ %}my.key{% endtrans %}

So it changes 4 spaces after to 1 space after.

And then with skipIfNewLine=false it should do the following:

 {%
 
     trans with {
     contactUrl: 'http://contact',
- }
- 
-    %}my.key{% endtrans %}
+ } %}my.key{% endtrans %}

It should take the 4 spaces + the 2 newlines, and convert that to 1 space.

[ruudk]

I think \TwigCsFixer\Tests\Rules\AbstractRuleTestCase::checkRule can be improved.

It should not immediately fail the test when one of the diffs fails. It should print the diff, then continue with showing the diff for the violations, and at the end fail it. This way, when you are trying to create a failing test, you can see the violations too. Now, I had to comment that part out, in order to see what the violations would be.

[VincentLanglet]

So I think what we want is that with skipIfNewLine=true (default) the following will be changed:

 {%
 
     trans with {
     contactUrl: 'http://contact',
 }
 
-    %}my.key{% endtrans %}
+ %}my.key{% endtrans %}

Not really, with skipIfNewLine let untouched this code because there is a new line before the last non empty token and %}. This allow to

• Add a new line
• Indent your code after the new line

And then with skipIfNewLine=false it should do the following:

 {%
 
     trans with {
     contactUrl: 'http://contact',
- }
- 
-    %}my.key{% endtrans %}
+ } %}my.key{% endtrans %}

It should take the 4 spaces + the 2 newlines, and convert that to 1 space.

It's understandable. That also mean that if you have only one space, new lines will be removed

 {%
 
     trans with {
     contactUrl: 'http://contact',
- }
- 
-  %}my.key{% endtrans %}
+ } %}my.key{% endtrans %}

[ruudk]

It's understandable. That also mean that if you have only one space, new lines will be removed

I think this is what I want with this feature. See newlines also as whitespace and enforce it regardless.

I never write

{%
  if var == true 
%}

or

{% 
trans into 'nl'
%}my.key{%endtrans %}

Am I missing something? Is this common?

Also, this rule is not default enabled, so we can make this different?

[VincentLanglet]

It should be ok with the latest commit 33fb7bb @ruudk

[ruudk]

@VincentLanglet Amazing 🎉 Thank you!

[VincentLanglet]

Thanks