Skip to content

"Convert upper/lower" case rule does not consider first statement in global class #92

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
suynwa opened this issue Aug 16, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@suynwa
Copy link

suynwa commented Aug 16, 2023

Our team uses the option "Derive from first statement" of the Convert upper/lower case rule. We do this because we work mostly with legacy code and want to keep the formatting settings of the original author.

However, i found a quirky bug when running the rule on global class:

class ZCL_TEST definition
  public
  final
  create public .

  public section.
    METHODS foo.
  protected section.
  private section.
endclass.



CLASS ZCL_TEST IMPLEMENTATION. " I think that SE80 converts this to all-CAPS
  method FOO.
    data VAR1 type I.
    data VAR2 type I.

    final(VAR3) = VAR1 + VAR2.
  endmethod.

ENDCLASS.

When i run the abapCleaner on the lines-of-code of FOO( ), it formats everything to CAPS.

  method FOO.
    DATA VAR1 TYPE I.
    DATA VAR2 TYPE I.

    " TODO: variable is assigned but never used (ABAP cleaner)
    FINAL(VAR3) = VAR1 + VAR2.
  endmethod.

My expectation was that the abapCleaner derives the formatting settings from the class ZCL_TEST definition line which are:

  • keywords in lower case
  • identifiers in UPPER CASE

The ABAP formatter (aka Pretty Printer) works as expected.

Cheers,
Suhas

@suynwa suynwa changed the title "Convert upper/lower" rule does not consider first statement in global class "Convert upper/lower" case rule does not consider first statement in global class Aug 16, 2023
@jmgrassau
Copy link
Member

Hi Suhas,

with "Derive from first statement", ABAP cleaner indeed separately determines the casing from

  • the first statement in a class definition (i.e. the CLASS ... DEFINITION line itself)
  • the first statement in a class implementation (i.e. the CLASS ... IMPLEMENTATION line itself)

The idea behind this is that in class definitions, SE80 always seems to use lower case keywords and upper case identifiers (regardless of the settings in menu "Utilities / Settings" -> ABAP Editor -> Pretty Printer), while for class implementations, SE80 does observe these settings, so SE80 could produce different formatting for the implementations (e.g. upper case keywords and lower case identifiers).

Therefore, if "derive from first statement" would now only derive the settings from the CLASS … DEFINITION line and someone in the team still uses SE80, you would always end up to have everything in lower case keywords and upper case identifiers – and that might not be intended, even by the SE80 users.

But you're right: SE80 does seem to put CLASS <NAME> IMPLEMENTATION to upper case! That makes this line rather useless to derive the settings from, of course. I think the best way to solve this would be to derive the settings for the implementation part from the first "METHOD method_name" line.

If that's still not satisfying, we could of course consider to add another option to the "Auto-determine upper/lower case" selection, so everyone could decide for themselves:

  • "derive from first statement in code document" (as ABAP Formatter does)
  • "derive from first statement of a class definition / method implementation"

Kind regards,
Jörg-Michael

@jmgrassau
Copy link
Member

Hi Suhas,

so now with
image

the implementation section depends on the first "METHOD …" line, therefore you'd get:

image

Kind regards,
Jörg-Michael

@jmgrassau jmgrassau added bug Something isn't working and removed new option labels Sep 4, 2023
@jmgrassau jmgrassau self-assigned this Sep 4, 2023
@suynwa
Copy link
Author

suynwa commented Sep 4, 2023

"derive from first statement of a class definition / method implementation"
This is promising 🥇

@jmgrassau
Copy link
Member

Hi Suhas,

thanks again for this hint! The renamed option and adjusted behavior was now released with version 1.5.3, hope it helps!

Kind regards,
Jörg-Michael

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