abap-cleaner icon indicating copy to clipboard operation
abap-cleaner copied to clipboard

Legacy Routines considered?

Open VladGhitulescu opened this issue 2 years ago • 13 comments

Hey,

I enjoy using ABAP Cleaner but have seen some inconsistencies working in legacy code: The settings from "Standardize empty lines within methods" doesn't seem to apply to routines in includes.

Is this by design?

Even when OO is the way to go, there is still A LOT of legacy code that would benefit from ABAP Cleaner.

Thanks!

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 04:11 VladGhitulescu

Hi Vlad,

thanks a lot! Happy to say that I don't have to work on legacy code that much, but yes, there is a lot of it out there, and if ABAP cleaner can help making it more readable, that would definitely be worth the effort.

Not sure here which ABAP statements you mean with "routines in includes": "Standardize empty lines within methods" should already work on typical report event blocks such as INITIALIZATION, AT SELECTION-SCREEN, START-OF-SELECTION etc., and of course on FORM ... ENDFORM and FUNCTION … ENDFUNCTION. Maybe you could give an example of what we're missing?

Kind regards, Jörg-Michael

jmgrassau avatar Nov 09 '23 07:11 jmgrassau

Hi JM

and thanks for the quick reply!

Here are the settings for it:

CleanShot 2023-11-09 at 09 02 02

and here is my routine

CleanShot 2023-11-09 at 09 00 21

missing all these starting/ending lines.

Hope is clearer now?

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 08:11 VladGhitulescu

Hi Vlad,

ah, thanks for the example! Actually this is working as expected, because "Max. empty lines" really means that the rule only removes superfluous empty lines, but never adds them (regardless of whether METHOD or FORM is used) – note that in the example code for the rule, the "incoming" code on the left-hand side already has the empty lines. That corresponds to the Clean ABAP Styleguide demanding to Add a single blank line to separate things, but not more.

We could of course add another option like "Min. empty lines" if you want enforce empty lines. In the case of FORM, that would esp. make sense if the FORM declaration spans multiple lines. (Actually, the new "Align FORM declarations" rule has two options for that).

On the other hand, you may want that to depend on the implementation, too, because if a method simply forwards a call to an attribute…

  METHOD any_method.

    mo_attribute->any_method( ).

  ENDMETHOD.

… empty lines may be overdoing it a bit.

Kind regards, Jörg-Michael

jmgrassau avatar Nov 09 '23 09:11 jmgrassau

Hi JM,

definitely you're right about one-liner.

However the rule DELETED my empty lines so I think is not only a "new option" but also a "bug".

What do you think?

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 09:11 VladGhitulescu

Hi Vlad,

ah, now I get it! Okay, I think in this case actually the rule "Align FORM declarations" is responsible for this, because of this option:

image

If you deactivate that, the empty line after FORM should not be removed. I cannot reproduce removal of the empty line above ENDFORM, though.

Hm, maybe these options regarding empty lines after FORM declarations should better be moved to the "Standardize empty lines within methods" rule, and "Align FORM declarations" should make sure that this rule is (re-)executed if the number of lines has changed (usually, rules are executed in the order shown in the rules list, so alignment always comes last).

Kind regards, Jörg-Michael

jmgrassau avatar Nov 09 '23 09:11 jmgrassau

Hi JM,

I'm somehow missing the rule "Align FORM declarations" completely

CleanShot 2023-11-09 at 10 25 18

Yes, your idea sounds good.

Thanks again!

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 09:11 VladGhitulescu

Hi Vlad,

those rules were added in with release 1.10.0 on 1.11.2023, see Release Notes – try the Eclipse menu "Help / Check for Updates"!

On the other hand, if you did not have this rule yet, it is indeed odd that this line was removed with your settings. Can you try to open the interactive cleanup with a FORM ... ENDFORM example, select the lines of FORM (+ next statement) and check what the "Rules Used in Current Selection" list says?

image

Kind regards, Jörg-Michael

jmgrassau avatar Nov 09 '23 09:11 jmgrassau

Hi JM,

strange things happen: After the update (thanks for the hint - "Align FORM declarations" is here now ;-)

CleanShot 2023-11-09 at 10 50 57

running ABAP Cleaner deletes only the line after FORM declaration and let the one above ENDFORM live.

Here is how it looks in the interactive cleanup:

CleanShot 2023-11-09 at 10 48 17

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 09:11 VladGhitulescu

Hi Vlad,

okay, then we're back to explainable behavior! I think if you now uncheck the last option in "Align FORM declarations", "Remove empty line after one-line FORM declaration", you'll get the intended results.

Nevertheless, let's keep this open for moving these options to where they belong.

Kind regards, Jörg-Michael

jmgrassau avatar Nov 09 '23 10:11 jmgrassau

Hi JM,

yesss! Thank you very much for your patience ;-) and support!

While unchecking the last option in "Align FORM declarations" I was wondering if is there a more direct way to access the configuration of ABAP Cleaner - right now I have to choose "Clean Up With Interactive ABAP Cleaner" first even when I don't want to clean up only to change something in the configuration (sorry for the OT question!!!).

Yes it would be great if this option will move.

Thanks again!

Regards, Vlad

VladGhitulescu avatar Nov 09 '23 10:11 VladGhitulescu

I was wondering if is there a more direct way to access the configuration of ABAP Cleaner - right now I have to choose "Clean Up With Interactive ABAP Cleaner" first even when I don't want to clean up only to change something in the configuration

I've had that thought too. I think it would be great if the ABAP Cleaner settings could be accessed through Eclipse's "Preferences" menu.

ConjuringCoffee avatar Nov 09 '23 11:11 ConjuringCoffee

I was wondering if is there a more direct way to access the configuration of ABAP Cleaner - right now I have to choose "Clean Up With Interactive ABAP Cleaner" first even when I don't want to clean up only to change something in the configuration

I've had that thought too. I think it would be great if the ABAP Cleaner settings could be accessed through Eclipse's "Preferences" menu.

… yes, as it should! ;-)

VladGhitulescu avatar Nov 09 '23 12:11 VladGhitulescu

Let's move this to a dedicated issue. I've created #191 for this purpose.

ConjuringCoffee avatar Nov 09 '23 12:11 ConjuringCoffee