-
Notifications
You must be signed in to change notification settings - Fork 70
[TRAN] Add new object type TRAN #713
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?
Conversation
Hi colleagues, |
Hi Colleagues, |
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.
Hi Yvonne, thanks for contributing to our abap-file-formats repository. TRAN is a huge file format, so please understand if we need multiple iterations of reviews here. Below, I've added some first comments. Please have a look and let me know what you think :)
Hi Marcus, thank you very much for reviewing our AFF. All the field names and titles are based on our data model. I need to confirm this with Peter Dell, but he's on vacation until tomorrow. |
Hi Yvonne, the AFF naming are separate from the data model names. In the aff persist class you can then map both. One of the main advantages of the AFF is providing a readable object, which does not happens if there are acronyms everywhere. Just to keep in mind ;) |
@WDFYvonne From my perspective, there are still some open points on your side. If you have made some changes, please upload the files again and let me know, when I can review again. If a review-comment is addressed from your point of view, please resolve the conversation. |
@Markus1812 we have closed all open points and upload the corrected AFF again. Please check it again. |
Hi Yvonne, thanks for the update. I've asked someone from my team to co-review your AFF. |
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.
Thank you for your contribution, @WDFYvonne.
Markus asked me to take a look at this pull request as well. Overall, the structure seems pretty solid. I've given some feedback on it, too.
However, I'm having some trouble with the names you've chosen for the AFF. I understand you've adapted them to fit your internal structure, but I want to emphasize that AFF is meant to be easily understood by users [1]. The names you choose should be as close as possible to what end users are familiar with. This applies not only to field titles but also to the field names themselves. These names appear in the source code and version management tools, like in git repositories or comparison editors. It's important to have user-friendly field names and titles for a better user experience.
An example is the difference between the field name platinMode and "SAP GUI for Java".
I recommend aligning these names with user terminology whenever possible. It might be helpful to finalize these field names during the UX review and then update them in the AFF afterward.
[1] https://github.com/SAP/abap-file-formats#background-and-scope
TYPES ty_class_program_name TYPE c LENGTH 40. | ||
"! <p class="shorttext">Class</p> | ||
"! Class name | ||
TYPES ty_class_name TYPE c LENGTH 30. |
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 you like, you could use following type for char 30 object names:
(z)if_aff_types_v1=>ty_object_name_30
.
We could even introduce a type for 40 character object names (if needed) for ty_class_program_name
.
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.
ok, I can also use if_aff_types_v1=>ty_object_name_30. We use the type c length 40 also for report and program name. Maybe it would be better to provide a general type with length 40?
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 you want to use it, we can provide it. I think object type with 40 characters might make sense to provide in general. Shall we go ahead with this?
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.
yes, please
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 have introduced a new type in (z)if_aff_types_v1 for object names with length 40 ("ty_object_name_40"). You can use it for your report/program name
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.
thank you :-)
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.
closed