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.

Sign up for GitHub

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

Jump to bottom

[SUCO] Add new object type SUCO #662

Merged
merged 13 commits into from
Oct 29, 2024
Merged

[SUCO] Add new object type SUCO #662

merged 13 commits into from
Oct 29, 2024

Conversation

WDFdaniel
Copy link
Contributor

No description provided.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Sep 25, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Sep 25, 2024

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.
You have signed the CLA already but the status is still pending? Let us recheck it.

@albertmink albertmink mentioned this pull request Sep 30, 2024
Merged
Markus1812
Markus1812 reviewed Sep 30, 2024
View reviewed changes
Copy link
Member

@Markus1812 Markus1812 left a comment

Choose a reason for hiding this comment

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

Hi Daniel, thanks for your AFF. I've added some comments, please have a look. In regard to the failing GitHub Action: We are working on it.

file-formats/suco/type/zif_aff_suco_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/suco/type/zif_aff_suco_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/suco/examples/zsu01_test12.suco.json Outdated Show resolved Hide resolved
@Markus1812 Markus1812 added the new-object This is a new object type added to AFF label Oct 1, 2024
@albertmink albertmink mentioned this pull request Oct 1, 2024
Merged
schneidermic0
schneidermic0 reviewed Oct 8, 2024
View reviewed changes
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 quite good from my point of view.

I had only one question and I think the issue reported by @GuilhermeSaraiva96 has not been solved, yet.

file-formats/suco/type/zif_aff_suco_v1.intf.abap Outdated Show resolved Hide resolved
schneidermic0
schneidermic0 reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good for me, now. I think this PR is ready to be reviewed by UX.

You might want to double check my question regarding the ABAP language version.

file-formats/suco/type/zif_aff_suco_v1.intf.abap Show resolved Hide resolved
@schneidermic0 schneidermic0 added the ux-review ready AFF is ready for UX review label Oct 17, 2024
@schneidermic0 schneidermic0 mentioned this pull request Oct 18, 2024
Closed
wurzka
wurzka reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@wurzka wurzka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

UX review has been performed for SUCO without any findings.

As soon as header is changed to the new type, we can merge this pull request

wurzka
wurzka approved these changes Oct 25, 2024
View reviewed changes
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, looks good to me 👍

schneidermic0
schneidermic0 approved these changes Oct 28, 2024
View reviewed changes
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, too 👍

@schneidermic0 schneidermic0 merged commit 14188e9 into SAP:main Oct 29, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuilhermeSaraiva96 GuilhermeSaraiva96 GuilhermeSaraiva96 left review comments

@Markus1812 Markus1812 Markus1812 left review comments

@wurzka wurzka wurzka approved these changes

@schneidermic0 schneidermic0 schneidermic0 approved these changes

Assignees
No one assigned
Labels
new-object This is a new object type added to AFF ux-review ready AFF is ready for UX review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@WDFdaniel @Markus1812 @schneidermic0 @wurzka @GuilhermeSaraiva96