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

Question: Replacing MOVE statement does not work as separate rule? #120

Closed
m-badura opened this issue Sep 28, 2023 · 6 comments
Closed

Question: Replacing MOVE statement does not work as separate rule? #120

m-badura opened this issue Sep 28, 2023 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@m-badura
Copy link

The rule Replace obsolete MOVE ... TO ... with = (1) seems to not cover all cases, when used separately from Unchain into multiple statements (2) and Simplify a chain with one element (3).

Rule (1) will clean this code:
MOVE: 1 to integer_1, 2 TO integer_2.

This code will be cleaned only when rule (2) was selected too:
MOVE: 1 to integer_3.

This code will be cleaned only when rule (3) was selected too:
MOVE 1 TO: integer_4, integer_5.

Since this behavior seems very unintuitive, maybe it could be changed? And if not, at least documented by rule (1)?

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

Hi m-badura,

you're right, this indeed didn't work as intended! Actually, the option …

image

… was introduced at some point in order to make this cleanup rule independent of the other two rules, but not all cases were covered then. Now this should be fixed in the next release:

image

Kind regards,
Jörg-Michael

@jmgrassau
Copy link
Member

Hi m-badura,

thanks again for opening this issue! This should now be fixed with version 1.6.0, which was just released.

Kind regards,
Jörg-Michael

@m-badura
Copy link
Author

m-badura commented Oct 11, 2023

Hi Jörg-Michael,
sorry for getting back on this to You, but I'm afraid the rule is still not 100 % accurate...

Here are examples:

MOVE 1: TO integer.
MOVE 1 TO integer:.
MOVE 1: TO integer_1, integer_2.
MOVE 1 TO integer:,.
MOVE EXACT: 1 to integer.
MOVE EXACT 1: TO integer.
MOVE EXACT 1 TO integer:.
MOVE EXACT: 1 TO integer_1, 2 TO integer_2.
MOVE EXACT 1: TO integer_1, TO integer_2.
MOVE EXACT 1 TO integer:,.
MOVE object_1: ?TO object_2.
MOVE object_1 ?TO object_2:.
MOVE object_1: ?TO object_2, ?TO object_3
MOVE object_1 ?TO object_2:,.

@jmgrassau
Copy link
Member

Hi m-badura,

thanks for stress-testing this rule :-)

Modifying your examples a bit, so it's easier to see what happens (and fixing two issues in the third and penultimate line):

MOVE 1: TO integer.
MOVE 2 TO integer:.
MOVE 3: TO integer_1, TO integer_2.
MOVE 4 TO integer:,.
MOVE EXACT: 5 to integer.
MOVE EXACT 6: TO integer.
MOVE EXACT 7 TO integer:.
MOVE EXACT: 8 TO integer_1, 9 TO integer_2.
MOVE EXACT 10: TO integer_1, TO integer_2.
MOVE EXACT 11 TO integer:,.
MOVE object_a: ?TO object_1.
MOVE object_b ?TO object_1:.
MOVE object_c: ?TO object_1, ?TO object_2.
MOVE object_d ?TO object_1:,.

At least when activating "Unchain into multiple statements" and "Simplify a chain with one element", the result looks good to me:

image

But you're right, the new option within "Replace obsolete MOVE ... TO with =" doesn't get any of those. Will look into it!

Or maybe the solution should rather be to offer an additional option within "Unchain into multiple statements" with which obsolete commands like ADD, MOVE, TRANSLATE can be unchained, even if you don't want to unchain everything else? Then the dedicated "Replace obsolete …" rules wouldn't need to care about chains anymore.

Kind regards,
Jörg-Michael

@jmgrassau jmgrassau reopened this Oct 12, 2023
@m-badura
Copy link
Author

thanks for stress-testing this rule :-)

You're welcome! :)
But thanks for providing this tool, constant improving it and the communication with us, users, here on GitHub.

Modifying your examples a bit (...)

I will not work nights, I will not work nights, ... ;)

At least when activating "Unchain into multiple statements" and "Simplify a chain with one element", the result looks good to me:

Yes, of course! But then it should be documented by the Replace obsolete... rule.

Or maybe the solution should rather be to offer an additional option within "Unchain into multiple statements" with which obsolete commands like ADD, MOVE, TRANSLATE can be unchained, even if you don't want to unchain everything else? Then the dedicated "Replace obsolete …" rules wouldn't need to care about chains anymore.

You've got me for it!

@jmgrassau
Copy link
Member

Hi m-badura,

with version 1.9.0, which was just released, all your examples should now correctly be replaced!

Pondering this for a while, I did however find that "decentral" options are better. Therefore, in 8 rules in the "COMMANDS" group (marked in yellow here) …

image

… you will now find new options like:

image
image
image
image
image

These options will, in a pre-processing step, first unchain all these obsolete statements (or remove the colon from a "chain of one"), so the replacement then works on all of them. That means, however, that unchaining will include cases in which some (or even all) statements can not be replaced:

image

image

But I think replacing old or even obsolete syntax in as many cases of as possible is a higher gain than keeping such chains.

Thanks again for opening this issue and testing this rule so thoroughly!

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