- 
                Notifications
    You must be signed in to change notification settings 
- Fork 72
Add structured values for ATC check variants (CHKV) #721
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
base: main
Are you sure you want to change the base?
Add structured values for ATC check variants (CHKV) #721
Conversation
| Welp, apparently the automated compatibility check disagrees that adding these fields is compatible with a simple "Something was added". Perhaps it does not consider the required/non-required characteristics of added types correctly? | 
| @bjoern-jueliger-sap this check is not required for a reason 😄 This isn't to suggest that the check is incorrect, but there's a possibility it might be. | 
| @bjoern-jueliger-sap thanks for the pull request. | 
| 
 The current example is "real" in that it uses actual SAP-delivered checks, i.e. importing this into a current ABAP release would give a check variant you can actually execute. Is that required for the example? If yes, we cannot add the new structures yet because there are no checks that use this feature yet. | 
| 
 If there are no checks which use these new structures yet, then it is ok for me to keep the example chkv as it is now | 
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.
Thanks for contributing your changes to the CHKV file format. @huber-nicolas asked me to have a look into it. Generally, it looks good to me. I just added two comments regarding names and titles.
| BEGIN OF ty_structured_value_comp, | ||
| "! <p class="shorttext">Component Name</p> | ||
| "! Name of a component of a structure | ||
| "! $required | ||
| field TYPE string, | ||
| "! <p class="shorttext">Component Value</p> | ||
| "! Value of a component of a structure | ||
| "! $required | ||
| value TYPE string, | ||
| END OF ty_structured_value_comp, | 
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.
We usually recommend having the title (<p class="shorttext">...</p>) and the name of the component in sync or at least almost in sync (like value and Component Value). For field this is not the case. What do you think about renaming it to
| BEGIN OF ty_structured_value_comp, | |
| "! <p class="shorttext">Component Name</p> | |
| "! Name of a component of a structure | |
| "! $required | |
| field TYPE string, | |
| "! <p class="shorttext">Component Value</p> | |
| "! Value of a component of a structure | |
| "! $required | |
| value TYPE string, | |
| END OF ty_structured_value_comp, | |
| BEGIN OF ty_structured_value_comp, | |
| "! <p class="shorttext">Component Name</p> | |
| "! Name of a component of a structure | |
| "! $required | |
| name TYPE string, | |
| "! <p class="shorttext">Component Value</p> | |
| "! Value of a component of a structure | |
| "! $required | |
| value TYPE string, | |
| END OF ty_structured_value_comp, | 
or
| BEGIN OF ty_structured_value_comp, | |
| "! <p class="shorttext">Component Name</p> | |
| "! Name of a component of a structure | |
| "! $required | |
| field TYPE string, | |
| "! <p class="shorttext">Component Value</p> | |
| "! Value of a component of a structure | |
| "! $required | |
| value TYPE string, | |
| END OF ty_structured_value_comp, | |
| BEGIN OF ty_structured_value_comp, | |
| "! <p class="shorttext">Component Field</p> | |
| "! Name of a component of a structure | |
| "! $required | |
| field TYPE string, | |
| "! <p class="shorttext">Component Value</p> | |
| "! Value of a component of a structure | |
| "! $required | |
| value TYPE string, | |
| END OF ty_structured_value_comp, | 
Better proposals are welcome ;)
| "! <p class="shorttext">Parameter Structure</p> | ||
| "! A structured value of a parameter | ||
| structured_value TYPE ty_structured_value, | ||
| "! <p class="shorttext">Parameter Structure List</p> | ||
| "! A list of structured values of a parameter | ||
| structured_value_list TYPE ty_structured_value_list, | 
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.
Same here, but I'm struggling with making a better proposals. But maybe syncing it with value_list and value_range_list is a quick win:
| "! <p class="shorttext">Parameter Structure</p> | |
| "! A structured value of a parameter | |
| structured_value TYPE ty_structured_value, | |
| "! <p class="shorttext">Parameter Structure List</p> | |
| "! A list of structured values of a parameter | |
| structured_value_list TYPE ty_structured_value_list, | |
| "! <p class="shorttext">Parameter Structure</p> | |
| "! A structured value of a parameter | |
| structured_value TYPE ty_structured_value, | |
| "! <p class="shorttext">List of Parameter Structures</p> | |
| "! A list of structured values of a parameter | |
| structured_value_list TYPE ty_structured_value_list, | 
This adds structured values and lists of structured values as possible types for parameters of checks that are included in a CHKV variant, which will be supported in a future release by the ATC Check Variant editor.
These structured values are the JSON representation of arbitrary ABAP structures whose fields are themselves not structured (i.e. correspond to the existing "value" JSON type here). Lists of structured values are correspondingly the JSON representation of ABAP table types whose line type is such a structure.
The format version has not been increased since JSONs from the earlier version are still fully compatible, as only optional fields have been added to the possible items of the
parameterslist.