Skip to content

Bug: DATA declarations not aligned if HASHED|SORTED TABLE with compound key #129

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

Closed
stockbal opened this issue Oct 3, 2023 · 6 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@stockbal
Copy link
Contributor

stockbal commented Oct 3, 2023

Hi,

I have an issue that DATA declarations that contain TABLE declarations with compound keys are not aligned correctly.

Unformatted Code

        data action_name                  TYPE string.
    DATA      tags_dac         TYPE REF TO zcl_abaptags_tags_dac.
    DATA new_tag_map            TYPE HASHED TABLE OF ty_tag_map WITH UNIQUE KEY tag_name owner.
            DATA tagged_objects       TYPE zabaptags_tagged_object_t.
        DATA tagged_objects_db        TYPE            zif_abaptags_ty_global=>ty_db_tagged_objects.

Rule Config for Align Declarations

    {
      "ruleID": "ALIGN_DECLARATIONS",
      "isActive": true,
      "settingCount": 10,
      "settings": {
        "CondenseInnerSpaces": "1",
        "AlignNonChainsAction": "2",
        "FillPercentageToJustifyOwnColumn": "20",
        "ExecuteOnClassDefinitionSections": "1",
        "StructureAlignStyle": "1",
        "AlignChainAction": "1",
        "AlignAcrossEmptyLines": "1",
        "AlignStructureAction": "1",
        "MaxLineLength": "130",
        "AlignAcrossCommentLines": "1"
      }
    }

Expectation

    DATA action_name TYPE string.
    DATA tags_dac TYPE REF TO zcl_abaptags_tags_dac.
    DATA new_tag_map TYPE HASHED TABLE OF ty_tag_map WITH UNIQUE KEY tag_name owner.
    DATA tagged_objects TYPE zabaptags_tagged_object_t.
    DATA tagged_objects_db TYPE zif_abaptags_ty_global=>ty_db_tagged_objects.

Actual Result

    DATA action_name                  TYPE string.
    DATA      tags_dac         TYPE REF TO zcl_abaptags_tags_dac.
    DATA new_tag_map            TYPE HASHED TABLE OF ty_tag_map WITH UNIQUE KEY tag_name owner.
    DATA tagged_objects       TYPE zabaptags_tagged_object_t.
    DATA tagged_objects_db        TYPE            zif_abaptags_ty_global=>ty_db_tagged_objects.

Conclusion

So, its not that nothing is done by ABAP cleaner, but the result is not satisfactory. However if I remove one key component from the table declaration line, or if I comment/remove the table declaration line, then I would get the expected result as seen above.

I traced the problem to the following code block:

while (typeEnd != null && !typeEnd.isAnyKeyword("LENGTH", "DECIMALS", "VALUE", "READ-ONLY") && !typeEnd.textEqualsAny(".", ",")) {
// do not align table declarations with "WITH ... KEY ..." sections, because they usually should not be put on a single line;
// however, do accept the short cases of "WITH EMPTY KEY" and "WITH [UNIQUE | NON-UNIQUE] DEFAULT KEY" and "WITH [UNIQUE | NON-UNIQUE] KEY comp1"
if (typeEnd.isKeyword("WITH")
&& !typeEnd.matchesOnSiblings(true, "WITH", "EMPTY", "KEY")
&& !typeEnd.matchesOnSiblings(true, "WITH", TokenSearch.makeOptional("UNIQUE|NON-UNIQUE"), "DEFAULT", "KEY")
&& !typeEnd.matchesOnSiblings(true, "WITH", TokenSearch.makeOptional("UNIQUE|NON-UNIQUE"), "KEY", TokenSearch.ANY_IDENTIFIER, ",|.")) {
return null;
}
typeEnd = typeEnd.getNextSibling();
}

Kind regards,
Ludwig

@jmgrassau
Copy link
Member

Hi Ludwig,

yes, well observed – this is a bit of a functional gap: ABAP cleaner leaves the declaration block unchanged if it contains a table type with multiple specified key components, because this declaration would end up in one line, and in case you have many key components, that line might be too long.

As you can see from the code, though, short cases such as WITH EMPTY KEY and WITH [UNIQUE] KEY <comp1> are accepted, and maybe it would already help to allow for some more components, e.g. comp1, comp2, comp3? In longer cases, I think it would anyway make sense to declare a table type TYPES ty_th_tag_map TYPE HASHED TABLE OF ... and then use it with DATA new_tag_map TYPE ty_th_tag_map.

Kind regards,
Jörg-Michael

@stockbal
Copy link
Contributor Author

stockbal commented Oct 6, 2023

Hi Jörg-Michael,

I guess that is a matter of taste. Sometimes I declare an extra table type even if I only use it just once, and sometimes I don't.
I think I could personally live with the table type workaround - if it would work. I made a rewrite of my initial declaration block:

    types ty_tag_map_table        type HASHED        TABLE OF ty_tag_map WITH UNIQUE KEY tag_name owner.

          DATA        action_name TYPE string.
    DATA tags_dac         TYPE REF TO zcl_abaptags_tags_dac.
    DATA new_tag_map        TYPE     TY_TAG_MAP_TABLe.
    DATA tagged_objects         TYPE zabaptags_tagged_object_t.
    DATA tagged_objects_db TYPE         zif_abaptags_ty_global=>ty_db_tagged_objects.

now the DATA declarations get formatted correctly, but the table type stays as is

    TYPES ty_tag_map_table        TYPE HASHED        TABLE OF ty_tag_map WITH UNIQUE KEY tag_name owner.

    DATA action_name TYPE string.
    DATA tags_dac TYPE REF TO zcl_abaptags_tags_dac.
    DATA new_tag_map TYPE ty_tag_map_table.
    DATA tagged_objects TYPE zabaptags_tagged_object_t.
    DATA tagged_objects_db TYPE zif_abaptags_ty_global=>ty_db_tagged_objects.

A few tests showed that the table type is now preventing the complete TYPES declaration block from being formatted. So the problem just moved to another section.

Kind regards,
Ludwig

@jmgrassau jmgrassau self-assigned this Oct 10, 2023
@jmgrassau jmgrassau added the bug Something isn't working label Oct 10, 2023
@jmgrassau
Copy link
Member

Hi Ludwig,

I think I found a better solution now. The "Align declarations" rule still does not actively align WITH clauses (which could of course get rather complex with many components, secondary keys etc.), but at least this does not prevent aligning the rest anymore:

image

The approach is now that when a WITH clause is found,

  • simple cases like WITH EMPTY KEY, WITH DEFAULT KEY or WITH KEY one_single_component are put on the same line behind it
  • everything else is simply kept as it is, including line breaks and manual alignment

image

This means that you might have to manually move the WITH clauses left or right a bit, but at least manual alignment is not destroyed in these cases.

Kind regards,
Jörg-Michael

@stockbal
Copy link
Contributor Author

Hi Jörg-Michael,

that works for me 😅👍🏻.

Kind regards,
Ludwig

@jmgrassau
Copy link
Member

Hi Ludwig,

great, and thanks for bringing this up – this should now be fixed with version 1.7.0!

Kind regards,
Jörg-Michael

@songzc3
Copy link

songzc3 commented Oct 11, 2023

Nice work!

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

3 participants