SAP / abap-cleaner

ABAP cleaner applies 95+ cleanup rules to ABAP code at a single keystroke
Apache License 2.0
458 stars 49 forks source link

Alignment inconsistency with ABAP Pretty printer #162

Open Lightirius opened 1 year ago

Lightirius commented 1 year ago

Hi. There is a little inconsistency with the default ABAP pretty printer (ABAP 7.55) for functional method calls without EXPORTING keyword with the following settings: align parameters and components

Pretty printer indents with two spaces instead of four. It would be great to have an option to control the amount of spaces in that case or just follow pretty printer formatting For example ABAP pretty printer formatting:

lo_foo->foo_method(
  iv_parameter1 = abap_true
  iv_parameter2 = abap_false
  iv_parameter3 = VALUE #( )
  iv_parameter4 = abap_false
).

ABAP cleaner formatting:

lo_foo->foo_method(
    iv_parameter1 = abap_true
    iv_parameter2 = abap_false
    iv_parameter3 = VALUE #( )
    iv_parameter4 = abap_false
).
suynwa commented 1 year ago

The formatting option of abap-Cleaner conforms to the clean ABAP styleguide. Refer to this section

image

I don't see a reason to add another option to control this behavior. IMHO, add such options will only bloat the plug-in.

jmgrassau commented 1 year ago

Hi Lightirius,

that's an interesting finding – as far as I know, this option "Format Functional Method Calls" hasn't been around for so long in ABAP Formatter, has it?

image

I shall try to find out why it is just indenting by 2 spaces – along with the styleguide, I think it is more consistent to add 4, because in case you need EXPORTING, IMPORTING, CHANGING keywords, you get the parameters indented by 4 spaces again even with ABAP Formatter:

    any_method(
      EXPORTING
        iv_any = 1
      IMPORTING
        ev_any = lv_any ).

More importantly, IMHO, indenting by 2 spaces should be reserved to inner code blocks (i.e. the statements after IF, LOOP AT etc.), while lines that continue a multi-line statement should be indented with more than 2 spaces, so you can immediately see they belong together. (But that's of course a clean ABAP discussion, which is what the styleguide is for…)

Current ABAP Formatter behavior also conflicts with styleguide rule If you break, indent parameters under the call, because it creates the anti-pattern

    DATA(lv_any) = get_value(
      iv_any   = 1
      iv_other = lv_value ).

rather than keeping things right of the assignment operator (or even behind the method call), if line length allows:

    DATA(lv_any) = get_value(
                       iv_any   = 1
                       iv_other = lv_value ).

So, ABAP cleaner would have to choose between an inconsistency with the styleguide rules, or an inconsistency with the ABAP Formatter. (Or offer an option to let users choose, of course).

Kind regards, Jörg-Michael

jmgrassau commented 1 year ago

P.S. @suynwa: I agree, the number of options esp. in the "Align parameters and components" rule is already very long. On the other hand, if some in your team use ADT + ABAP cleaner, others SAP GUI + PrettyPrinter, you'd get "edit wars" about this – or there might even be some check that demands PrettyPrinter formatting (see #128). So in these cases, some compromise between ABAP cleaner and PrettyPrinter might be the best solution… at least until we've won over everyone for ADT ;-)

Lightirius commented 1 year ago

Hi Jörg-Michael,

In ADT release notes this option was added in ADT version 3.20. This version lists 7.56 as supported backend version, but 7.55 also works in both in SAP GUI and Eclipse too. It was added to backend somewhere between 7.52 and 7.55, because we got it after upgrade from 7.52 to 7.55 (I had to install a few additional notes on backend to fix some bugs though) It is possible that behaivior was changed in later backend releases or notes to align with official styleguides

Fell free to close this issue if it is considered minor inconsistency or fixed in later backend releases