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

[EDCK] Add new object type for EDCK #629

Merged
merged 29 commits into from
Oct 8, 2024

Conversation

KUMARMUKULSAP
Copy link
Contributor

No description provided.

Copy link

cla-assistant bot commented Jul 5, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Jul 5, 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.

Copy link

@GuilhermeSaraiva96 GuilhermeSaraiva96 left a comment

Choose a reason for hiding this comment

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

Hi, thank you for the contribution! Do not mind the amount of comments, they are basically all the same. We have some guidelines on how the title and description are written: sentence case for descriptions (only the very first letter is capital) (https://github.com/SAP/abap-file-formats/blob/main/docs/json.md#description)

file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
@GuilhermeSaraiva96
Copy link

Furthermore, the example object is missing

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

Thank you for resolving the comments above. We have looked over it again and found some minor issues again. Please take a look.

You are also missing an example. If you could add one with the name z_aff_example_edck.edck.json in an examples folder and link it in the README.md.

file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
@KUMARMUKULSAP
Copy link
Contributor Author

@Markus1812 : I have incorporated all above review comments. However, I am stuck at the check Generated Json Schema differs from provided ones. I am not really sure how to resolve this error. I cannot find out from where I can download the generated JSON schema to perform the comparison with the provided JSON schema. Please help.

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.

Took me some while to find the cause for the schema mismatch but with the first two suggestions it should work now.
The other two are some minor issues with title case.

In one of the commits, you committed a .DS_Store file. Can you please remove it again?

file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
@KUMARMUKULSAP
Copy link
Contributor Author

@Markus1812 : Thank you for helping out. I have incorporated the review comments. and deleted the .DS_Store file as well. Please approve.

@Markus1812
Copy link
Member

In case you have missed it: One comment above is still unresolved. There was not only the z missing of zif... missing but also the exist has to be existence_check to be the same as the constant name.

#629 (comment)

@KUMARMUKULSAP
Copy link
Contributor Author

@Markus1812 : Constant name of the default value has been corrected as well. Let me know if further changes are required.

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.

Looks good to me now, thank you. In the next step, the ux-review team will take over.

@Markus1812 Markus1812 requested a review from a team September 10, 2024 08:57
@Markus1812 Markus1812 added the ux-review ready AFF is ready for UX review label Sep 10, 2024
@wurzka wurzka removed the request for review from a team September 10, 2024 09:33
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.

Hi @KUMARMUKULSAP, please see my comments. In addition to the boolean we already discussed, I found some mismatches between title and name of the component

file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
@KUMARMUKULSAP
Copy link
Contributor Author

@wurzka : Requested changes are done.

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 updates @KUMARMUKULSAP!
Just one minor, else looks good to me.

file-formats/edck/type/zif_aff_edck_v1.intf.abap Outdated Show resolved Hide resolved
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 providing the last change. Looks good to me!

@schneidermic0 schneidermic0 merged commit 59b44ef into SAP:main Oct 8, 2024
10 checks passed
@schneidermic0
Copy link
Contributor

@KUMARMUKULSAP welcome as contributor :)

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 ux-review ready AFF is ready for UX review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants