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

Split BracesFixer #4884

Draft
wants to merge 3 commits into
base: 2.15
from

Conversation

@julienfalque
Copy link
Member

julienfalque commented Mar 16, 2020

Fixes #823 and #776.

TODO

  • implement a replacement for adding space between some keywords and parentheses (e.g. if (), could be #4435
  • Simplify implementation of some complicated fixers
  • Add tests for AlternativeSyntaxAnalyzer
  • Add more unit tests for fixers
@julienfalque julienfalque force-pushed the julienfalque:split-braces-fixer-2.15 branch 6 times, most recently from 7c53590 to 9202f6e Mar 16, 2020
@julienfalque julienfalque force-pushed the julienfalque:split-braces-fixer-2.15 branch from 9202f6e to 7bcb815 Mar 18, 2020
@julienfalque julienfalque requested review from SpacePossum and keradus Mar 18, 2020
@julienfalque julienfalque linked an issue that may be closed by this pull request Mar 18, 2020
/**
* @param bool $bracesFixerCompatibility
*/
public function __construct($bracesFixerCompatibility = false)

This comment has been minimized.

Copy link
@keradus

keradus Mar 24, 2020

Member

Please remove constructor parameter from this and other fixers.
Fixers should become services and receive only dependencies via constructor.

This look like configuration (the option should state what the trigger changes in behaviour of fixer, not if it's compat mode with old fixer or not)

This comment has been minimized.

Copy link
@julienfalque

julienfalque Mar 24, 2020

Author Member

I used these options to drop behaviors from braces that were too specific and opinionated IMO and that I didn't want to keep. The new fixers don't have those behaviors at all, unless executed via braces for backward compatibility.

Do you want to keep the braces specific behavior with the new fixers?

This comment has been minimized.

Copy link
@keradus

keradus Mar 24, 2020

Member

what are the specific behaviors of braces?
can't understand it by looking at bracesFixerCompatibility name :(
I think we should decide one by one.

Same time, fixer is public, please no constructor parameter like this.
If we would decide that sth is too weird and we keep it as compatibility only and not as a proper configuration option, then create a method like /** @internal */ enableBracesCompatibilityMode(): void {...}

This comment has been minimized.

Copy link
@julienfalque

julienfalque Mar 27, 2020

Author Member

Here is the list of braces specific behaviors, with test failures from BracesFixerTest that cover them.

1. In control_structure_braces: when a control structure has no braces and contains an alternative syntax control structure, do not add braces on the outer control structure
-<?php if ($a) foreach ($b as $c): ?> X <?php endforeach; ?>
+<?php if ($a) {
+    foreach ($b as $c): ?> X <?php endforeach;
+} ?>

Maybe we could improve this by using the alternative syntax on the outer control structure (but out of scope here I'd say):

-<?php if ($a) foreach ($b as $c): ?> X <?php endforeach; ?>
+<?php if ($a): foreach ($b as $c): ?> X <?php endforeach; endif; ?>
2. In blank_lines_inside_block: when a curly braces block starts with blank lines followed by a comment, don't remove the blank lines
 <?php
 if (true) {
-
     //  The blank line helps with legibility in nested control structures
     if (true) {
         // if body
     }
 
     // if body
 }
3. In statement_indentation: when a comment looks like commented-out code, don't indent it
 <?php
 function foo()
 {
     $a = 1;
-//    if ($a === 'bar') {
-//        return [];
-//    }
+    //    if ($a === 'bar') {
+    //        return [];
+    //    }
     // we will return sth
     return $a;
 }
4. In statement_indentation: when a single line comment is part of a multiline message, don't indent it
 <?php
 function foo()
 {
     $bar = 1;                   // multiline ...
-                                // ... comment
+    // ... comment
     $baz  = 2;                  // next comment
 }
5. In statement_indentation: assume code that is not inside any curly braces block is correctly indented
 <?php
-    if ($test) { //foo
-        echo 1;
-    }
+if ($test) { //foo
+    echo 1;
+}
keradus and others added 2 commits Mar 24, 2020
CS
@julienfalque julienfalque force-pushed the julienfalque:split-braces-fixer-2.15 branch from b157f90 to 68073c2 Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.