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

Align declarations inserts an unexpected number of spaces #14

Closed
fabianlupa opened this issue May 19, 2023 · 4 comments
Closed

Align declarations inserts an unexpected number of spaces #14

fabianlupa opened this issue May 19, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@fabianlupa
Copy link

Before

    TYPES: BEGIN OF al11_file,
             dirname  TYPE dirname_al11,
             name     TYPE filename_al11,
             type     TYPE c  LENGTH 10,
             len      TYPE p  LENGTH 8 DECIMALS 0,
             owner    TYPE fileowner_al11,
             mtime    TYPE p LENGTH 6 DECIMALS 0,
             mode     TYPE c LENGTH 9,
             useable  TYPE c LENGTH 1,
             subrc    TYPE c LENGTH 4,
             errno    TYPE c LENGTH 3,
             errmsg   TYPE c LENGTH 40,
             mod_date TYPE d,
             mod_time TYPE c LENGTH 8,
             seen     TYPE c LENGTH 1,
             changed  TYPE c LENGTH 1,
             status   TYPE c LENGTH 1,
           END OF al11_file,
           al11_file_tab TYPE STANDARD TABLE OF al11_file.
    FIELD-SYMBOLS <file_list> TYPE al11_file_tab.

After

    TYPES: BEGIN OF al11_file,
             dirname     TYPE dirname_al11,
             name        TYPE filename_al11,
             type        TYPE c                           LENGTH 10,
             len         TYPE p                           LENGTH 8 DECIMALS 0,
             owner       TYPE fileowner_al11,
             mtime       TYPE p                           LENGTH 6 DECIMALS 0,
             mode        TYPE c                           LENGTH 9,
             useable     TYPE c                           LENGTH 1,
             subrc       TYPE c                           LENGTH 4,
             errno       TYPE c                           LENGTH 3,
             errmsg      TYPE c                           LENGTH 40,
             mod_date    TYPE d,
             mod_time    TYPE c                           LENGTH 8,
             seen        TYPE c                           LENGTH 1,
             changed     TYPE c                           LENGTH 1,
             status      TYPE c                           LENGTH 1,
           END OF al11_file,
           al11_file_tab TYPE STANDARD TABLE OF al11_file.
    FIELD-SYMBOLS <file_list> TYPE al11_file_tab.
@fabianlupa
Copy link
Author

I found some other examples. It seems like the LENGTH addition is aligned after the type of the longest definition in the chain, therefore above STANDARD TABLE OF al11_file. Which I guess is fine if you know about it. Or the component in the type statement that is not part of the BEGIN OF / END OF could have been extracted to its own statement. Here's a more reasonable example of the same:

  TYPES:
    access_type           TYPE c          LENGTH 1,
    mode                  TYPE c          LENGTH 15,
    encoding              TYPE c          LENGTH 15,
    linefeed              TYPE c          LENGTH 10,
    endian                TYPE c          LENGTH 1,
    BEGIN OF mode_options,
      encoding            TYPE encoding,
      linefeed            TYPE linefeed,
      endian              TYPE endian,
      codepage            TYPE cpcodepage,
    END OF mode_options,
    BEGIN OF os_additions,
      type                TYPE string,
      filter              TYPE string,
    END OF os_additions,
    replacement_character TYPE c          LENGTH 1.

@fabianlupa
Copy link
Author

Also doesn't this directly implement the anti pattern listed in the Clean ABAP rule?

https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#dont-align-type-clauses

Independant statements are also aligned which is the exact example of what you are not supposed to do?

    DATA file_system      TYPE REF TO zabc_file_system.
    DATA path             TYPE REF TO zabc_path.
    DATA current_codepage TYPE cpcodepage.

jmgrassau added a commit to jmgrassau/abap-cleaner that referenced this issue May 21, 2023
@jmgrassau jmgrassau added the bug Something isn't working label May 21, 2023
@jmgrassau
Copy link
Member

Hi Fabian,

thanks a lot for these examples, this should also be fixed in version 1.2! Now, with default settings in "Align declarations", I get (original code -> cleaned code):

image

image

Also doesn't this directly implement the anti pattern listed in the Clean ABAP rule?

Yes, it indeed does – that's why the "Align declarations" rule is deactivated in the "essential" Profile (in which only those rules are active which are explicitly demanded by the style guide).

However, from my experience, this is a rather controversial rule in the style guide, so ABAP cleaner offers "Align declarations" to whoever likes it. And you could argue: If nobody wanted to align TYPEs, then why does Pretty Printer do it on chains, structures and METHODS declarations? Why does the style guide itself have many positive examples with aligned TYPEs? …

Kind regards,
Jörg-Michael

@fabianlupa
Copy link
Author

Thanks! My initial understanding of the tool was that the default settings do implement the rules of the Clean ABAP styleguide. After reading https://github.com/SAP/abap-cleaner/blob/main/docs/profiles.md I see the non-default Essential profile does that instead. I also found out what the "<->" means in the configuration dialog by reading that.

I do wonder though why this isn't the other way around as at least according to the guide itself it is the official reference guide for S/4HANA development, therefore I would have assumed most users want those settings.

Regarding the rule itself I have no opinion, especially when there's no manual work involved in following it. I just find the "alignment" / consistency between the tools (Code Pal, ADT, ABAP Cleaner) and guidelines (Clean ABAP GitHub, Clean ABAP Book) to be very important for adoption without frustration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants