Skip to content

fix: maintain area for replacing service #988

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix: maintain area for replacing service #988

wants to merge 1 commit into from

Conversation

ap891843
Copy link
Contributor

Signed-off-by: ap891843 [email protected]

@ap891843 ap891843 force-pushed the bugfix/ReplaceServiceMaintainArea branch 2 times, most recently from a0f868e to dd7472f Compare June 24, 2021 15:57
@ap891843 ap891843 marked this pull request as ready for review June 24, 2021 16:01
@ap891843 ap891843 force-pushed the bugfix/ReplaceServiceMaintainArea branch 2 times, most recently from b906dc1 to 7f3cce9 Compare June 25, 2021 06:13
@temanbrcom temanbrcom added the bug Something isn't working label Jun 25, 2021
temanbrcom
temanbrcom suggested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@temanbrcom temanbrcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message doesn't explain the UX of that fix. Maybe, it would be better to use "Maintain string length while replacing"?
Also, please, add a link to the issue.
BTW, does mapping work correctly?

@temanbrcom
Copy link
Contributor

@nalmabrcom please, run the UI tests on this PR

@ap891843 ap891843 force-pushed the bugfix/ReplaceServiceMaintainArea branch 2 times, most recently from dc2a0b2 to 43faf82 Compare June 25, 2021 09:22
temanbrcom
temanbrcom approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@temanbrcom temanbrcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦕

@ap891843
Copy link
Contributor Author

The commit message doesn't explain the UX of that fix. Maybe, it would be better to use "Maintain string length while replacing"?
Also, please, add a link to the issue.
BTW, does mapping work correctly?

I didn't find any mapping issue. The way I try to figure this is, place a debug at applyLookAhead and see if we land there in debug mode. Didn't land there :)

@temanbrcom temanbrcom linked an issue Jun 25, 2021 that may be closed by this pull request
Copy replacing does not correctly maintain areas #974
Closed
grianbrcom
grianbrcom reviewed Jun 25, 2021
View reviewed changes
server/src/test/java/org/eclipse/lsp/cobol/usecases/TestCopybookReplacePatterns.java Outdated
+ "1 PROGRAM-ID. TESTREPL.\n"
+ "2 DATA DIVISION.\n"
+ "3 WORKING-STORAGE SECTION.\n";
"0 IDENTIFICATION DIVISION.\r\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to change \n to \r\n.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D yes, I forgot to revert it. I was getting a weird issue later to find that the culprit was WorkspaceFileService.java .
But forgot to revert later on.

Copy link
Contributor Author

@ap891843 ap891843 Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have reverted some and kept some randomly. So that we test that it works for both.

...c/main/java/org/eclipse/lsp/cobol/core/preprocessor/delegates/util/ReplacingServiceImpl.java
}
}

private List<String> splitStringByLength(String actualStr, int fixedLength) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you change a number of strings and I'm afraid that this will change our mapping. I tried to reproduce the case when we need it and found this bug.
COBOL program:

0      IDENTIFICATION DIVISION.
1      PROGRAM-ID. TESTREPL.
2      DATA DIVISION.
3      WORKING-STORAGE SECTION.
       01  LOGA.  
       COPY REPL5  REPLACING ==LDAY== BY ==DMAN12345789sadfasfdsafasdfsa
      -    asdfsafsfabfgbtdndfrg==.

       PROCEDURE DIVISION.
           MOVE 0 TO DMAN12345789sadfasfdsafasdfsaasdfsafsfabfgbtdndfrg.

COPYBOOK REPL5.

      ***************************************************************** 09700000
         02  LOGHDR.                                                    18000000
            03  LDAY           PIC S9(7) COMP-3.

I see that after the preprocessor changed line with 03 LDAY is very long and not split. This also causes some errors to the user. Did you try to solve issues like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I tried something like this. I actually hoped that it won't give any mapping issues as the replacement with continued lines happens before we start the actual pre-processing. And the long replacement text is should be because the pre-processor concatenates. But I guess, I missed some scenarios. I will look into it. Thanks :)
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's interesting because you are showing the exact case I did, but you don't have any errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my mistake. I have added a use-case test for this scenario.

@temanbrcom temanbrcom changed the title fix: maintain area for replacing service. fix: maintain area for replacing service. [Frozen] Jun 25, 2021
@nalmabrcom
Copy link
Contributor

Results:
image

@ap891843 ap891843 force-pushed the bugfix/ReplaceServiceMaintainArea branch from 43faf82 to 2edaa91 Compare June 25, 2021 12:54
@temanbrcom temanbrcom changed the title fix: maintain area for replacing service. [Frozen] fix: maintain area for replacing service Jun 25, 2021
@ap891843
Copy link
Contributor Author

ap891843 commented Jun 29, 2021 •
edited
Loading

Closing as discussed.
A more efficient and better way to solve this issue would be to handle this in the pre-processor stage.

@ap891843 ap891843 closed this Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grianbrcom grianbrcom grianbrcom left review comments

@temanbrcom temanbrcom temanbrcom approved these changes

@asatklichov asatklichov Awaiting requested review from asatklichov

Assignees

@ap891843 ap891843

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copy replacing does not correctly maintain areas
4 participants
@ap891843 @temanbrcom @nalmabrcom @grianbrcom