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

[AIFD] Add new object type AIFD #612

Merged
merged 19 commits into from
Jul 1, 2024
Merged

[AIFD] Add new object type AIFD #612

merged 19 commits into from
Jul 1, 2024

Conversation

D047539
Copy link
Contributor

@D047539 D047539 commented May 3, 2024

No description provided.

Copy link

cla-assistant bot commented May 3, 2024

CLA assistant check
All committers have signed the CLA.

@albertmink albertmink added the new-object This is a new object type added to AFF label May 6, 2024
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.

See my first comments/questions below.

Do you think it is possible to provide an example? I guess this will help us to review the object type.

file-formats/aifd/README.md Outdated Show resolved Hide resolved
file-formats/aifd/type/zif_aff_aifd_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifd/type/zif_aff_aifd_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.

I am sorry for the late response. I did not manage to look into it earlier.

Thanks for providing the example. This was helpful for me. 👍

I try to first have a look at the general structure. Most probably I suggest to use different names, later as soon as you think the structure is fine.

file-formats/aifd/examples/z_aff_example_aifd.aifd.json 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.

I guess I understood your object's structure, now. This is fine for me.

I just found some minor issues/questions.

file-formats/aifd/type/zif_aff_aifd_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifd/type/zif_aff_aifd_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifd/type/zif_aff_aifd_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/aifd/examples/z_aff_example_aifd.aifd.json 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.

@D047539 Thanks for all your adaptations. In general it looks good to me. 👍

However, I am still struggling with the different interface names and object names and I am not sure whether I get your requirements. Do you think it makes sense to discuss this in a short call with you, @wurzka and me?

@schneidermic0 schneidermic0 self-requested a review May 17, 2024 07:30
@schneidermic0 schneidermic0 dismissed their stale review May 17, 2024 07:32

Open question has been clarified. Since I won't be available for the next two weeks, I will dismiss my review

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 adaption. Looks good to me, now.

@wurzka wurzka requested a review from a team May 17, 2024 16:01
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 your update. Looks good to me.

},
"aifInterfaces":[
{
"interfaceObjectName":"INT_OBJ_1"
Copy link
Contributor

Choose a reason for hiding this comment

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

@D047539 did you consider to change interfaceObjectName to name? Typically, the array name, here 'aifInterfaces', is sufficient and in particular if the array element contains a single String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@albertmink no, we have not changed the name from interfaceObjectName to name because name is very generic one and from our point of view it is not a clear name. We would like to keep the interfaceObjectName as name.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this has been suggested beforehand, I just wanted to make sure that this has been considered by you. You are allowed to overrule our suggestions 😐

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.

LGTM

wurzka and others added 2 commits July 1, 2024 09:34
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
@wurzka wurzka merged commit 6891815 into SAP:main Jul 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-object This is a new object type added to AFF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants