[SITO] File format for situation objects - New #380

micotto · 2022-07-21T11:10:13Z

No description provided.

schneidermic0

Thanks, @micotto. I had a first look at your pull request. I think there are a lot of improvements compared to #364. 👍

I went through the proposal (I put focus on the example in the first step) and added a bunch of questions, comments and suggestions.

I guess there will be more in further iterations. This file format definition is one of the largest so far. ;)

file-formats/sito/examples/zmo_sito_test.sito.json

schneidermic0 · 2022-07-22T07:33:14Z

file-formats/sito/examples/zmo_sito_test.sito.json

+        ],
+        "structureSemanticKey": [
+          {
+            "fieldOrder": "02",


Does the field fieldOrder represent the order of semantic keys? This seems to be similar to the discussion of order of db table fields for indexes. There we take the approach to not specify an order field. The order is specified by the order of entries in the array.

Relates to #176

Field Order is used to set the order for the concatenation of keys for the semantic key. It is a critical information. If the explicit definition of the field order is removed and the order is just defined by the implicit order of the fields in the array, a user could miss the necessity/importance of that order. Without the explizit field order definition it just looks like an array of keys where the order does not matter!

file-formats/sito/examples/zmo_sito_test.sito.json

file-formats/sito/type/zif_aff_sito_v1.intf.abap

schneidermic0

We suggest to write titles/shorttexts in title case and descriptions in ABAPdoc comments in standard sentence case.

schneidermic0 · 2022-07-25T10:04:56Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+      "! <p class="shorttext">In Memory</p>
+      "! In Memory


Usually, the tile stays in title case. The descriptions below are in standard sentence case.

Suggested change

"! <p class="shorttext">In Memory</p>

"! In Memory

"! <p class="shorttext">In Memory</p>

"! In memory

This seems to be the case for almost all descriptions

Adapted for all descriptions

Adapted for all descriptions

It seems not to be fixed, yet.

test change

cla-assistant · 2022-07-26T11:15:20Z

All committers have signed the CLA.

cla-assistant · 2022-07-26T11:15:21Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ schneidermic0
✅ micotto
❌ tobiasmuench
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…ormats

micotto · 2022-07-26T13:28:00Z

We went through all review comments. For some, files/code were adapted. Others we would like to clarify. Please see our comments above.

schneidermic0 · 2022-07-26T14:35:52Z

We went through all review comments. For some, files/code were adapted. Others we would like to clarify. Please see our comments above.

Shall we already review your changes? Or shall we wait until you clarified the other points?

schneidermic0

You didn't change this file by intension, did you?

docs/json.md

tobiasmuench · 2022-07-27T11:43:54Z

Regarding the two different versions of header type, ty_header_60 and ty_header_60_cloud.
ADT tooling as of my knowledge is ABAP Langeuage Version space (SAP Development) and 5 (Customer Development) only. ABAP Language Version 2 (Key User Extensibility) is reserved for Fiori UIs.

Why do we have to chose the one or other type? What impact does it have or should we clarify this with our cloud enablement coach? (Currently we got no further feedback from him about open tasks regarding Cloud Enablement.) We do offer Fiori tools for ABAP Language Version 2 already and are now building ADT to cover the other dev scenarios.

Summary..: YES, Situation Objects (as well as Situation Scenarios and Templates) are subject to Key User Extensibility and Fiori Apps for ABAP Language Version 2 are already delivered.
Thus I expect TYPE zif_aff_types_v1=>ty_header_60 is relevant for us!

tobiasmuench · 2022-07-27T12:56:15Z

@schneidermic0
We consider our input now as final, subject for final review!
Only recommendation that we didn't follow is the field order, see explanation above.

marcushoepfner · 2022-07-28T12:54:59Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+      "! $required
+      type       TYPE ty_sit2_do_vh_type,
+      "! <p class="shorttext">Scope</p>
+      "! Source


Field is named "scope", while description says "Source". Is this correct?
I wonder since "scope" might mean "source" as well, but also has other meanings.

for me both are fine, just wanted to point you to.

Will be changed to "SCOPE" entirely!

wurzka

Puh, a very long type. I have to admit that I have lost track a bit... However, my review is focused on the titles and descriptions. These are only suggestions. It's up to you if you want to implement them

file-formats/sito/type/zif_aff_sito_v1.intf.abap

schneidermic0

@micotto and @tobiasmuench: Thanks. Good progress!

We added a lot of feedback (most of them are minor changes due to inconsistencies in titles and descriptions).

Maybe it make sense to talk about following general remarks/ideas in a call/meeting:

Order of fields in structures (especially for required and boolean fields)
Reuse of types
Field field_order in semantic keys

file-formats/sito/type/zif_aff_sito_v1.intf.abap

schneidermic0 · 2022-07-29T09:51:47Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+      "! <p class="shorttext">In Memory</p>
+      "! In Memory


Adapted for all descriptions

It seems not to be fixed, yet.

schneidermic0 · 2022-07-29T09:59:09Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+  "! <p class="shorttext">SIT2_DO_SCOPE</p>
+  "! SIT2_DO_SCOPE


Minor: Do you need the DDIC types, here? At leaset for the title (shorttext) I would remove them.

There are several places

@SAP/abap-file-formats-team Can we handle @links in the description of ABAP doc? If yes, they could add a link to the ddic type by adding {@link SIT2_DO_SCOPE}. This would be helpful in ADT's element info. What do you think?

not sure what is meant here

I guess @schneidermic0 suggests to remove at least the titles which only consist of the DDIC types. Do you need these DDIC types here?

About the @link: I tried to add a link to SIT2_DO_SCOPE in the description. It does not work.

So for you @micotto:
If you need the DDIC type information here, you can think about having it only in the description, i.e. remove the "shorttext"`s (here and for the types and constants below).

If you don't need the information, I would remove it.

file-formats/sito/type/zif_aff_sito_v1.intf.abap

schneidermic0 · 2022-07-29T11:26:57Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+
+    "! <p class="shorttext">Semantic Keys</p>
+    "! Semantic keys of an object structure
+    ty_sit2_obj_str_sk_list TYPE STANDARD TABLE OF ty_sit2_obj_str_sk WITH DEFAULT KEY,


If you want to keep the field field_order, should the entries be sorted by the field_order?

for the moment, we are fine with keeping field field_order, based on standard table
(we tried to use sorted table, but we obtain a syntax error in if_aff_sito_v1)

If you want to have a sorted table, it should be possible to have it like

Suggested change

ty_sit2_obj_str_sk_list TYPE STANDARD TABLE OF ty_sit2_obj_str_sk WITH DEFAULT KEY,

ty_sit2_obj_str_sk_list TYPE SORTED TABLE OF ty_sit2_obj_str_sk WITH UNIQUE KEY field_order,

tobiasmuench · 2022-08-09T10:57:15Z

Draft of user story:
https://jira.tools.sap/browse/SITUATIONS-3679

tobiasmuench · 2022-08-09T12:11:15Z

Order of fields in structures (especially for required and boolean fields)

Order of fields is definitely important and will be fixed!

Order proposal:

Id
Name
Type
Reusable
Scope
SAP object node type
.... sonstige nodes ...

micotto · 2022-08-22T07:20:01Z

Hi colleagues,
thank you for your thorough review.
We tried to address all of your comments. Regarding field "field_order", we kept it since we believe it is important to make it explicit for the user.

wurzka · 2022-08-23T11:00:58Z

Thanks for your update @micotto. I am taking over for @schneidermic0.
But before I start my review, could you please mark our suggestions either as "resolved" or put a comment if or if not you adopted our suggestions? This makes my review quite easier since I do not have to ask if you thought about these suggestions. Thanks :)

…ormats

micotto · 2022-08-23T14:11:05Z

Hi @wurzka. I went through the review comments once more and adapted everything which was missing. With a few exceptions (where I put comments), all suggestions are resolved now.

wurzka

Thanks for your update. I added two comments. Please give a short feedback whether you want to do any of the changes.

wurzka · 2022-08-24T08:42:09Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+  "! <p class="shorttext">SIT2_DO_SCOPE</p>
+  "! SIT2_DO_SCOPE


I guess @schneidermic0 suggests to remove at least the titles which only consist of the DDIC types. Do you need these DDIC types here?

About the @link: I tried to add a link to SIT2_DO_SCOPE in the description. It does not work.

So for you @micotto:
If you need the DDIC type information here, you can think about having it only in the description, i.e. remove the "shorttext"`s (here and for the types and constants below).

If you don't need the information, I would remove it.

wurzka · 2022-08-24T08:45:12Z

file-formats/sito/type/zif_aff_sito_v1.intf.abap

+
+    "! <p class="shorttext">Semantic Keys</p>
+    "! Semantic keys of an object structure
+    ty_sit2_obj_str_sk_list TYPE STANDARD TABLE OF ty_sit2_obj_str_sk WITH DEFAULT KEY,


If you want to have a sorted table, it should be possible to have it like

Suggested change

ty_sit2_obj_str_sk_list TYPE STANDARD TABLE OF ty_sit2_obj_str_sk WITH DEFAULT KEY,

ty_sit2_obj_str_sk_list TYPE SORTED TABLE OF ty_sit2_obj_str_sk WITH UNIQUE KEY field_order,

micotto · 2022-08-25T10:32:07Z

We'll do both suggested changes.
Regarding the DDIC types, by "removing title" do you mean keeping a line such as no. 4 with empty title or deleting the complete line ?
Also, does your suggestion apply to the TYPES-statements only, or to the CONSTANTS-statements as well ?

wurzka · 2022-08-25T11:08:01Z

I suggest deleting the complete line s.t. there is only line

"! SIT2_DO_SCOPE

I would remove these titles also for the constants.

micotto · 2022-08-25T13:35:52Z

Both suggested changes were carried out.
In addition, we had to change two entities from array to item (plus one tiny change were a plural was missing.)

wurzka

Thanks. Looks good to me, now. I think we can merge the pull request.

Not available to review again. Review was taken over by myself.

Initial object upload

3afb4c1

micotto changed the title ~~Initial object upload~~ [SITO] File format for situation objects - New Jul 21, 2022

micotto added 4 commits July 21, 2022 14:39

Resolve abaplint issue

c547859

Resolve abaplint issue 2

1dc8584

Resolve abaplint issue 3

602be5c

Resolve abaplint issue 4

4e60cb6

schneidermic0 mentioned this pull request Jul 21, 2022

[SITO] File format for situation objects #364

Closed

schneidermic0 self-requested a review July 22, 2022 07:03

schneidermic0 requested changes Jul 22, 2022

View reviewed changes

schneidermic0 requested changes Jul 25, 2022

View reviewed changes

schneidermic0 and others added 2 commits July 25, 2022 12:11

Merge branch 'main' into feature/sito

a85e402

Update json.md

2df7822

test change

micotto added 2 commits July 26, 2022 14:53

SITO: Adapt entity names, ID, descriptions

d43fc73

Merge branch 'feature/sito' of https://github.com/micotto/abap-file-f…

86eaa8f

…ormats

Merge branch 'main' into feature/sito

73f1811

schneidermic0 requested changes Jul 26, 2022

View reviewed changes

docs/json.md Outdated Show resolved Hide resolved

schneidermic0 self-requested a review July 28, 2022 08:24

marcushoepfner reviewed Jul 28, 2022

View reviewed changes

wurzka reviewed Jul 28, 2022

View reviewed changes

schneidermic0 previously requested changes Jul 29, 2022

View reviewed changes

schneidermic0 mentioned this pull request Aug 3, 2022

Arrays/internal tables, sorting #176

Open

6 tasks

Revert docu changes

e37d6ce

Descriptions, order, etc.

ec1516e

Merge branch 'main' into feature/sito

44adf6d

micotto requested a review from schneidermic0 August 23, 2022 09:05

micotto added 2 commits August 23, 2022 15:59

Singular vs Plural

6f0b8fb

Merge branch 'feature/sito' of https://github.com/micotto/abap-file-f…

63d3341

…ormats

wurzka reviewed Aug 24, 2022

View reviewed changes

Merge branch 'main' into feature/sito

c6acf96

DDIC, SORTED, NAV+CB

2472e48

wurzka approved these changes Aug 25, 2022

View reviewed changes

wurzka merged commit 843a9d4 into SAP:main Aug 25, 2022

	ty_sit2_obj_str_sk_list TYPE STANDARD TABLE OF ty_sit2_obj_str_sk WITH DEFAULT KEY,
	ty_sit2_obj_str_sk_list TYPE SORTED TABLE OF ty_sit2_obj_str_sk WITH UNIQUE KEY field_order,

[SITO] File format for situation objects - New #380

[SITO] File format for situation objects - New #380

Conversation

micotto commented Jul 21, 2022

schneidermic0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneidermic0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-assistant bot commented Jul 26, 2022 • edited Loading

cla-assistant bot commented Jul 26, 2022

micotto commented Jul 26, 2022

schneidermic0 commented Jul 26, 2022

schneidermic0 left a comment

Choose a reason for hiding this comment

tobiasmuench commented Jul 27, 2022 • edited Loading

tobiasmuench commented Jul 27, 2022

marcushoepfner Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wurzka left a comment

Choose a reason for hiding this comment

schneidermic0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasmuench commented Aug 9, 2022

tobiasmuench commented Aug 9, 2022 • edited Loading

micotto commented Aug 22, 2022

wurzka commented Aug 23, 2022

micotto commented Aug 23, 2022

wurzka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micotto commented Aug 25, 2022 • edited Loading

wurzka commented Aug 25, 2022

micotto commented Aug 25, 2022

wurzka left a comment

Choose a reason for hiding this comment

cla-assistant bot commented Jul 26, 2022 •

edited

Loading

tobiasmuench commented Jul 27, 2022 •

edited

Loading

marcushoepfner Jul 28, 2022 •

edited

Loading

tobiasmuench commented Aug 9, 2022 •

edited

Loading

micotto commented Aug 25, 2022 •

edited

Loading