-
Notifications
You must be signed in to change notification settings - Fork 62
Fix cics extract #2463
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
Fix cics extract #2463
Conversation
Drafted to wait for #2464 |
81e3d55
to
78d7a66
Compare
server/engine/src/main/antlr4/org/eclipse/lsp/cobol/implicitDialects/cics/CICSParser.g4
Show resolved
Hide resolved
...main/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSExtractOptionsUtility.java
Outdated
Show resolved
Hide resolved
...main/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSExtractOptionsUtility.java
Outdated
Show resolved
Hide resolved
…dded in missed duplicate options, length required only with SET
…ies found that are not the first or are not the same token type as the first to remove redundancy with checkDuplicates
...in/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSOptionsCheckBaseUtility.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSOptionsCheckBaseUtility.java
Outdated
Show resolved
Hide resolved
...main/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSExtractOptionsUtility.java
Outdated
Show resolved
Hide resolved
...main/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSExtractOptionsUtility.java
Outdated
Show resolved
Hide resolved
checkHasMutuallyExclusiveOptions("INTO or SET", ctx.INTO(), ctx.SET()); | ||
if (!ctx.SET().isEmpty()) | ||
checkHasExactlyOneOption("LENGTH or FLENGTH", ctx, ctx.cics_length_flength()); | ||
checkHasMutuallyExclusiveOptions("MAXLENGTH or MAXFLENGTH", ctx.cics_maxlength()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be checkCicsMaxlength function, that internally calls checkHasMutuallyExclusiveOptions("...", ctx.MAXLENGTH(), ctx.MAXDLENGTH())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach allows the use of getClass()
that you mentioned above. Will try it out and change the generic List<E>
to be a List<TerminalNode>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ctx.FROM().isEmpty()) checkHasIllegalOptions(ctx.LENGTH(), "LENGTH"); | ||
if (ctx.TERMINAL().isEmpty()) checkHasIllegalOptions(ctx.INPARTN(), "INPARTN"); | ||
if (ctx.FROM().isEmpty()) checkHasIllegalOptions(ctx.LENGTH(), "LENGTH without FROM"); | ||
checkHasMutuallyExclusiveOptions("INTO or SET", ctx.cics_into_set()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something similar here, I'd expect checkIntoSet(ctx.cics_into_set())
and then in it, length check + checkHasMutuallyExclusiveOptions("...", ctx.get(0).INTO(), ctx.get(0).SET())
or maybe the forEach approach as in the ISSUE
command.
...main/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSExtractOptionsUtility.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSOptionsCheckBaseUtility.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSOptionsCheckBaseUtility.java
Outdated
Show resolved
Hide resolved
|
||
// Only raise error if this validates mutual exclusivity and is not an artifact of duplicate | ||
// options | ||
if (!nodes.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were not allowed to use Stream due to it being Java 9?
|
||
List<TerminalNode> children = new ArrayList<>(); | ||
|
||
Stream.of(rules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More use of Stream
* @param rules Lists of TerminalNode to iterate through | ||
* @return Number of TerminalNode instances found | ||
*/ | ||
protected int checkHasMutuallyExclusiveOptions(String options, List<TerminalNode>... rules) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No generic and check for class? What about if we want to enforce sets of ParserRuleContext variables inconjunction with TerminalNodes as well?
I had the best of both worlds set up, but the PRs are going slowly. See my commit here: development...mm-broadcom:che-che4z-lsp-for-cobol:CICS-WEB-Command-Fix-V2#diff-50eac5e66277f894df3f0b69a3d008dfc1aa9d9f0460c5c3c1b5ff41898af1f7R203
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mm-broadcom - We kinda landed on the idea of using a custom checkXYZContext()
for all custom context's in the CheckXYZOptionsCheckUtility
subclasses where we "unpack" each context manually for mutual exclusivity. However, checkHasExactlyOneOption()
persists the usage of mixed type passing for both TerminalNodes
and ParserRuleContexts
adds mutually exclusive options checking
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: