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

Fix CICS Allocate #2454

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

chacebot
Copy link
Contributor

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

  • TestCICSAllocate.java
  • Tests tested against COBOL compiler

Checklist:

  • Each of my commits contains one meaningful change
  • I have performed rebase of my branch on top of the development
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@chacebot
Copy link
Contributor Author

chacebot commented Sep 4, 2024

@slavek-kucera when you get a chance

Comment on lines 238 to 240
cics_allocate_appc_mro_lut61_sysid: SYSID cics_data_area (PROFILE cics_name | NOQUEUE | STATE cics_cvda | cics_handle_response)*;
cics_allocate_lut61_session: SESSION cics_name (PROFILE cics_name | NOQUEUE | cics_handle_response)*;
cics_allocate_appc_partner: PARTNER cics_name (NOQUEUE | STATE cics_cvda | cics_handle_response)*;
Copy link
Contributor

Choose a reason for hiding this comment

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

the preprocessor seems to tolerate reordering the SYSID, SESSION and PARTNER keywords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am aware, this is a good topic for design discussion. This ordering is under the assumption that the user would start these commands off with at least the target sub command option - it seems unusual not to. However it does not make us fully flexible but does clean up our routing efficiency through the antlr parser. Am happy to revert to the (...)+ style if we want this to be FULLY flexible to chunk the target subcommand anywhere in the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would also bring up the argument to question if we should pull in the main command to these i.e:
(ALLOCATE | SYSID ...)+

Copy link
Contributor

Choose a reason for hiding this comment

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

ALLOCATE is required to be before the other arguments, however those could be all part of the same (...)+ rule, verified in the visitor.

Comment on lines 42 to 51
private static final String APPC_ALL_OPTIONS_VALID_ONE =
ALLOCATE + PARTNER + NOQUEUE + STATE.trim();

private static final String APPC_ALL_OPTIONS_VALID_TWO =
ALLOCATE + SYSID + PROFILE + NOQUEUE + STATE.trim();

private static final String APPC_INVALID_ONE =
ALLOCATE + PARTNER + "{PROFILE|error1}(100) " + NOQUEUE + "STATE(err)";

private static final String LUT61_ALL_OPTIONS_VALID_ONE =
ALLOCATE + SESSION + PROFILE + NOQUEUE.trim();

private static final String LUT61_INVALID_ONE =
ALLOCATE + "SESSION(100) " + "PROFILE(100) " + "{STATE|errorOne}(100)";

private static final String MRO_ALL_OPTIONS_VALID_ONE =
ALLOCATE + SYSID + PROFILE + NOQUEUE + STATE.trim();

Copy link
Contributor

Choose a reason for hiding this comment

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

These constants are actually making it harder for me to follow the test. I suggest moving the concatenations directly into the test bodies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, will update.

@slavek-kucera slavek-kucera merged commit d4de416 into eclipse-che4z:development Sep 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants