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

Error when comments in the middle of a command MOVE. #147

Closed
openPhiL opened this issue Oct 13, 2023 · 3 comments
Closed

Error when comments in the middle of a command MOVE. #147

openPhiL opened this issue Oct 13, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@openPhiL
Copy link

Hi guys,
I have this reproduceable issue where the cleaner cannot handle code that has comments in the middle, similar to #88

this works:

REPORT zvzxphu_1.
DATA lv_werks TYPE werks_d.
MOVE 5 TO 
lv_werks.

this does not:

REPORT zvzxphu_1.
DATA lv_werks TYPE werks_d.
MOVE 5 TO "test
lv_werks.

error in the log reads:
20231013_165445 , line 12: Failed referential integrity test on command starting at source line 12!

@openPhiL openPhiL changed the title Error when comments in the middle of a command. Error when comments in the middle of a command MOVE. Oct 13, 2023
@jmgrassau jmgrassau added the bug Something isn't working label Oct 14, 2023
@jmgrassau jmgrassau self-assigned this Oct 14, 2023
@jmgrassau
Copy link
Member

Hi openPhil,

thanks for reporting this! Yes, comments, pragmas or colons in unexpected places are annoying… but this should be fixed in the next release.

To identify more of these issues, I now implemented a "stress test" for both the unit tests of ABAP cleaner and the >30 MB of real-life ABAP code on which I run ABAP cleaner before every release. With the stress test, each code document is now additionally tested 128 times, and every time,

  1. either a) " line-end comments, or b) * comment lines, or c) ## pragmas, or d) : colons are introduced after
  2. each 1st / 2nd / 3rd / … 32nd token (if syntactically allowed) of each ABAP statement.

Then all cleanup rules are executed on the code, and the test checks whether referential integrity is kept. So you could say, the stress test now cleans approx. 4000 MB of code with lots of comments, pragmas, and colons in all sorts of weird places.

This did find a few more issues which will all be fixed in the next release; and the stress test will of course check regression for further development efforts.

Kind regards,
Jörg-Michael

@openPhiL
Copy link
Author

Hi,
I think this is a great approach to generalize the issue!
One thought is left though.. Your approach (at least what I understand) assumes that it is enough to have 1 (as in single / as in once) element of 1. for each of 2.
technically, we can have them all combined.

concatenate ##needed
*put advertisement here:
lv_a ##needed lv_b into: ##needed "really
data(lv_c), data(lv_x).

(this beautiful code doesn't break anything (yet), it is just to explain me :)
There might be edge cases that have multiple pragmas and comments combined in some lines.

@jmgrassau
Copy link
Member

Hi openPhiL,

this should now be fixed with version 1.7.2, which was just released!

And you're right, it would be a nice enhancement of the stress test to introduce multiple elements into the same statement. Of course, to some extent, this already happens, because the element added by the stress test comes on top of what comments, pragmas, and colons are already there. But I'll think of something here! Since there are too many possible combinations, there may be a stress test variant that adds multiple elements in random places (just have to think how to persist changes in case of errors in order to make them reproducible).

Kind regards,
Jörg-Michael

P.S.: I absolutely like *put advertisement here 😉: code is indeed among the very few places in our lives that are still advertisement-free, so there's certainly great potential here!

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