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

[COTA] Add new object type COTA #543

Merged
merged 24 commits into from
Sep 21, 2023
Merged

[COTA] Add new object type COTA #543

merged 24 commits into from
Sep 21, 2023

Conversation

WUEHR
Copy link
Contributor

@WUEHR WUEHR commented Jul 20, 2023

No description provided.

@cla-assistant
Copy link

cla-assistant bot commented Jul 20, 2023

CLA assistant check
All committers have signed the CLA.

@albertmink
Copy link
Contributor

@WUEHR typically objects don't store its changedby or changedat and others. This is considered meta data and redundant as explained https://github.com/SAP/abap-file-formats#background-and-scope

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.

@WUEHR Thanks for your pull request

See my first comments below.

In general, please try to use understandable field names, because the file format is intended to be human readable. Usually we recommend to use the same field name you would choose for a field label on an UI (see https://github.com/SAP/abap-file-formats/blob/main/README.md#background-and-scope).

Please, also try to fix the issues reported by abaplint. Thanks.

file-formats/cota/cota/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/zif_aff_cota_v1.intf.json Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_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 updates.

Please see my comments. I guess a bunch of fields can still be removed.

If this is done, you might want to come up with a grouping of your fields.

file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_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 all improvements. I have added some more questions

file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_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.

Basically, it looks good for me, now. However, some more issues to fix:

  1. I think the folder structure is still broken. There are two nested folders cota. (Example: file-formats/cota/cota/README.md)
    I think this is also the reason for the failing JSON validation check.
  2. Remove the example link from the readme or add an example to solve the failing link check
  3. Descriptions should be sentence style (see examples below and check the others, too)

file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
schneidermic0
schneidermic0 previously approved these changes Jul 26, 2023
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, @WUEHR. Looks good to me, now.

@schneidermic0 schneidermic0 requested a review from a team July 26, 2023 10:30
@schneidermic0 schneidermic0 self-requested a review July 28, 2023 15:09
@schneidermic0 schneidermic0 dismissed their stale review July 28, 2023 15:11

A new open question appeared: enum with only one value

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.

See my comments :)

file-formats/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
@wurzka wurzka self-requested a review August 3, 2023 06:25
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 update @WUEHR.
There are still some description issues.

file-formats/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/cota/type/zif_aff_cota_v1.intf.abap Outdated Show resolved Hide resolved
@wurzka wurzka self-requested a review August 3, 2023 08:50
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.

LGTM :) (from UX point of view)

@WUEHR
Copy link
Contributor Author

WUEHR commented Aug 15, 2023

We will have a discussion with RFC colleagues when Michael Acker is back from vacation -> please don't merge as long as the meeting did not take place

Thank you :)

@schneidermic0
Copy link
Contributor

We will have a discussion with RFC colleagues when Michael Acker is back from vacation -> please don't merge as long as the meeting did not take place

Thank you :)

Were you able to discuss the topic? How shall we proceed?

@WUEHR
Copy link
Contributor Author

WUEHR commented Sep 12, 2023

Hello colleagues,

the possibility of a language version change is not working yet but it is being registered by the language version team in the moment, I followed https://wiki.one.int.sap/wiki/display/ApplServ/ABAP+Language+Version+Development+Guideline.
How is it possible to make fields visible depending on the language version? I haven't found anything about that in the docus.

Thanks and best regards
Frank

@WUEHR
Copy link
Contributor Author

WUEHR commented Sep 13, 2023

We've already talked about it: Showing and hiding individual fields depending on other fields will be possible at some point soon, right?

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
Copy link
Contributor

@WUEHR Welcome as contributor 🎉

@schneidermic0 schneidermic0 merged commit 1d4e1f2 into SAP:main Sep 21, 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