Skip to content

[GSMG] Add new object type #593

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
Draft
wants to merge 58 commits into
base: main
Choose a base branch
from
Draft

[GSMG] Add new object type #593

wants to merge 58 commits into from

Conversation

mseich
Copy link
Member

@mseich mseich commented Mar 15, 2024

No description provided.

@albertmink albertmink added the new-object This is a new object type added to AFF label Mar 15, 2024
albertmink
albertmink reviewed Mar 15, 2024
View reviewed changes
wurzka
wurzka requested changes Mar 18, 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.

Hi @mseich, thanks for uploading the GSMG file format! Please see my comments below.
I'm afraid I didn't express myself precisely enough with regard to the descriptions when we talked about it. They should be in sentence case, but the titles (the shorttexts) in title case.

file-formats/gsmg/examples/z_aff_example_gsmg.gsmg.json Outdated
},
"generalInformation": {
"type": "USAGE",
"detailedDescription": "This Metric Group is for testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this description about? Is it an extension of the description field in the header? So like the name says, is it for describing the object in more detail than the header-description allows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly right :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just some additional questions: Did you thought about extending the standard description to 255 characters. As far as I know, this should be possible.

If it is more a documentation. Did you consider to provide documentation using KTD?

mseich and others added 16 commits March 19, 2024 10:18
@mseich @albertmink
Update file-formats/gsmg/examples/z_aff_example_gsmg.gsmg.json
a777188 
Co-authored-by: Albert Mink <albert.mink@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
2898e05 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
e0b5e18 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
5bbb486 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
b7a4c18 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
1fe7cad 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/type/zif_aff_gsmg_v1.intf.abap
a13a41b 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich @wurzka
Update file-formats/gsmg/README.md
13820b5 
Co-authored-by: Katharina Wurz <katharina.wurz@sap.com>
@mseich
Update zif_aff_gsmg_v1.intf.abap
4b2edfa 
Switched type to category
@mseich
Update zif_aff_gsmg_v1.intf.json
071a0fe 
Typo
@mseich
Update gsmg-v1.json
ca1ca16
@mseich
Update z_aff_example_gsmg.gsmg.json
ec6075d
@mseich
Update zif_aff_gsmg_v1.intf.abap
ce1b698
@mseich
EOL Unix style (LF)
222c9c8
@mseich
Get rid of enums
752ac82
@mseich
Auto enums
7c34be4
@mseich mseich requested review from schneidermic0 and wurzka March 22, 2024 09:55
schneidermic0
schneidermic0 requested changes Mar 22, 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.

In general, it looks quite good. Just some minor issues.

Please see also my question regarding the detailedDescription. I followed-up on @wurzka's question. (see above)

file-formats/gsmg/examples/z_aff_example_gsmg.gsmg.json Outdated
},
"generalInformation": {
"type": "USAGE",
"detailedDescription": "This Metric Group is for testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just some additional questions: Did you thought about extending the standard description to 255 characters. As far as I know, this should be possible.

If it is more a documentation. Did you consider to provide documentation using KTD?

albertmink and others added 4 commits March 26, 2024 17:23
@albertmink
Merge branch 'main' into feature/gsmg
a4778d7
@mseich @schneidermic0
Update file-formats/gsmg/README.md
880cba2 
Co-authored-by: Michael Schneider <micha.schneider@sap.com>
@mseich @schneidermic0
Update file-formats/gsmg/examples/z_aff_example_gsmg.gsmg.json
7d6f46d 
Co-authored-by: Michael Schneider <micha.schneider@sap.com>
@mseich
exceptions -> exception
e8a7d73
schneidermic0
schneidermic0 reviewed Mar 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.

Thanks for your update.

Did you see my question related to the detailedDescription field?

schneidermic0
schneidermic0 approved these changes Apr 17, 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.

Thanks, @mseich. Looks good to me, now.

@schneidermic0 schneidermic0 requested a review from a team May 7, 2024 13:49
@albertmink albertmink added the ux-review ready AFF is ready for UX review label Jul 22, 2024
@wurzka wurzka removed the request for review from a team September 10, 2024 09:34
@schneidermic0
Copy link
Contributor

@mseich @albertmink As far as I can see the UX cross check did took place. I see only one open question. Should the order of co_category be changed to an alphabetical order?

If it is fine as it is now (non-alphabetic), I think, we could merge this PR.

@wurzka wurzka marked this pull request as draft November 13, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schneidermic0 schneidermic0 schneidermic0 approved these changes

@wurzka wurzka Awaiting requested review from wurzka

@albertmink albertmink Awaiting requested review from albertmink

Requested changes must be addressed to merge this pull request.

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.

4 participants

@mseich @schneidermic0 @albertmink @wurzka