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

[SIAD]Add file format #572

Merged
merged 4 commits into from
Dec 6, 2023
Merged

[SIAD]Add file format #572

merged 4 commits into from
Dec 6, 2023

Conversation

aron-leibfried
Copy link
Contributor

No description provided.

Copy link

cla-assistant bot commented Nov 24, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Nov 24, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Leibfried seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

"! <p class="shorttext">Business Role Template ID</p>
"! Business Role Template ID
"! $required
BusinessRoleTemplateID TYPE aps_iam_brt_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

we prefer primitive data types (towards self contained, assuming basic types as char are known), so aps_iam_brt_id is a char[30] right?

Copy link
Contributor

@albertmink albertmink left a comment

Choose a reason for hiding this comment

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

Some minor casing issues to start with.

file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thanks for the update 👍

Just one more question and one more minor comment. Otherwise it looks good to me.

"! <p class="shorttext">Business Role Template ID</p>
"! Business role template ID
"! $required
business_role_template_id TYPE c LENGTH 30,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is fine for me. Just two minor remarks:

  1. In most cases, we call it object names (instead of their ID). Maybe, we can even skip it at all (I guess this would be my preferred solution) However, I would take this to the UX review what they think.
  2. As mentioned before, for object names with the length of 30 we already provide a generic type which you can reuse: zif_aff_types_v1=>ty_object_name_30 (or if_aff_types_v1=>ty_object_name_30 within SAP systems)

Comment on lines 36 to 39
"! <p class="shorttext">General</p>
"! General
"! $required
assignment TYPE ty_assignment,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you want to emphasise. Do developers specify "general information" or the "assignments" of the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We compared our editor with other editors of us and we always have this "General" at top in other editors.
But you are right developers specify more the "assignments" of the object, so "Assignment" would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, "General Information" is used, if we are not able to find a better semantic name. If there is a more specific name, we suggest to use this one. See second-last paragraph of https://github.com/SAP/abap-file-formats/blob/main/docs/json.md#writing-json-schema-with-abap-types

I am not sure, maybe, it's even "Assignements" because there are multiple assignments (to the business role template and to the space template)? It's not an assignment between business role template and space template, is it? What do you think?

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 think general information would be the best choice, the whole object is an assignment of a business role template and a space template, so if it would be "Assignment(s)" this would be a duplicate and makes not that much sense in this position.

Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@schneidermic0 schneidermic0 requested a review from a team November 27, 2023 10:29
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

We performed the UX review. See my comments.

file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/siad/type/zif_aff_siad_v1.intf.abap Outdated Show resolved Hide resolved
@albertmink albertmink changed the title Add SIAD file format [SIAD]Add file format Dec 6, 2023
Copy link
Contributor

@wurzka wurzka left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Looks good to me!

Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thanks, @aron-leibfried. Welcome as contributor! :)

@schneidermic0 schneidermic0 merged commit 7937dfe into SAP:main Dec 6, 2023
8 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.

4 participants