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

[SCP1]Add default value and rename constant name #573

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

goreraks
Copy link
Contributor

@goreraks goreraks commented Dec 6, 2023

Refers to #396

Copy link

cla-assistant bot commented Dec 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Dec 6, 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.
You have signed the CLA already but the status is still pending? Let us recheck it.

file-formats/scp1/scp1-v1.json Outdated Show resolved Hide resolved
:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [`z_aff_example_scp1.scp1.json`](./examples/z_aff_example_scp1.scp1.json)
:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [z_aff_example_scp1.scp1.json](./examples/z_aff_example_scp1.scp1.json)

Choose a reason for hiding this comment

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

Suggested change
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [z_aff_example_scp1.scp1.json](./examples/z_aff_example_scp1.scp1.json)
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [`z_aff_example_scp1.scp1.json`](./examples/z_aff_example_scp1.scp1.json)

Choose a reason for hiding this comment

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

An there too much spaces which have been added but I cannot add them to my suggestion. You may remove them manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has to be fixed in the report because I can see that same spaces are available in the file generated from report.

Choose a reason for hiding this comment

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

Ok... can you please fix it anyway? Also the "`" was removed

:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [`z_aff_example_scp1.scp1.json`](./examples/z_aff_example_scp1.scp1.json)
:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [z_aff_example_scp1.scp1.json](./examples/z_aff_example_scp1.scp1.json)

Choose a reason for hiding this comment

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

An there too much spaces which have been added but I cannot add them to my suggestion. You may remove them manually

file-formats/scp1/scp1-v1.json Outdated Show resolved Hide resolved
@albertmink
Copy link
Contributor

@goreraks the check Validate JSON schema / validate (pull_request) is not a mandatory/required test. So in the first place, you can ignore it. For us, as the infrastructure provider, this is a meaningful test ;)

// cc. @marcushoepfner

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.

Thank you for the update! I have some small suggestions related to your changes.

If you adapt these, the schema needs to be generated, again.

file-formats/scp1/type/zif_aff_scp1_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/scp1/type/zif_aff_scp1_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 think we can still change SCP1 incompatible, because there is no usage of it (e.g., in abapGit), yet.

Therefore, Looks good to me.

:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [`z_aff_example_scp1.scp1.json`](./examples/z_aff_example_scp1.scp1.json)
:--- | :--- | :--- | :--- | :---
`<name>.scp1.json` | 1 | [`zif_aff_scp1_v1.intf.abap`](./type/zif_aff_scp1_v1.intf.abap) | [`scp1-v1.json`](./scp1-v1.json) | [z_aff_example_scp1.scp1.json](./examples/z_aff_example_scp1.scp1.json)

Choose a reason for hiding this comment

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

Ok... can you please fix it anyway? Also the "`" was removed

@marcushoepfner
Copy link

@schneidermic0 can you pls. double check and approve if ok?

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! But I want to wait merging the changes until the tests were executed

@albertmink
Copy link
Contributor

Merging and overruling the checks.

@albertmink albertmink merged commit 2538729 into SAP:main Dec 13, 2023
8 checks passed
@goreraks goreraks deleted the updateSCP1FileFormat branch December 15, 2023 09:00
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