[SITO] File format for situation objects #364

tobiasmuenchsap · 2022-05-12T14:52:06Z

Initial pre-review version of SITO AFF

Please get in contact with us for review appointment

Initial pre-review version of SITO AFF

cla-assistant · 2022-05-12T14:56:56Z

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.
1 out of 2 committers have signed the CLA.

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

huber-nicolas · 2022-05-12T15:44:38Z

file-formats/sito/sito-v1.json

+          "type": "string",
+          "maxLength": 32
+        },
+        "abapLanguageVersion": {


The abap_lanuage_version is saved in the header-part. No need to save it here

huber-nicolas · 2022-05-12T15:47:44Z

file-formats/sito/sito-v1.json

+          "title": "Object Extensible",
+          "description": "Extensibility Flag for Situation Objects",
+          "type": "string",
+          "enum": [


It looks to me that for this field "type": "boolean" is sufficient. You should only use an enum if you think that in future some more values will be available.

In our interface, a boolean representation is CHAR(1) - as usual in ABAP.

How can we avoid that it is transformed to string instead of boolean?

You can just use abap_bool as type. This will be transferred to boolean in the JSON schema.

Here you can see an example:

abap-file-formats/file-formats/clas/type/zif_aff_clas_v1.intf.abap

Line 71 in d371351

fix_point_arithmetic TYPE abap_bool,

We should update the documentation. I have created an issue see #365

huber-nicolas · 2022-05-12T15:48:46Z

file-formats/sito/sito-v1.json

+        "sitnobjisreusable": {
+          "title": "Object Reusable",
+          "description": "Reusability Flag",
+          "type": "string",


I think "type": "boolean" is sufficient here

huber-nicolas · 2022-05-12T15:56:51Z

file-formats/sito/sito-v1.json

+              "sitnobjstrucisreusable": {
+                "title": "Object Reusable",
+                "description": "Reusability Flag",
+                "type": "string",


is "type": "boolean" sufficient?

huber-nicolas · 2022-05-12T15:59:13Z

file-formats/sito/sito-v1.json

+                  "properties": {
+                    "sitnobjstrucdesc": {
+                      "title": "Situation Object Structure Description",
+                      "description": "Situation Object Structure Description",


Please write the description in sentence case. This means to write the description as if it was a normal english sentence. Example: "The quick brown fox jumps over the lazy dog"

We extract these texts from our Data Elements Domains. Our UA experts want them to be camel case. Would it be really required to parse and change the texts for the json serialization?

In general, titles shall be title style, descriptions shall be sentence style

However, for names like here, we usually stick to the definition in SAP term. I guess your UAs know whether they are written upper case. ;)
So in your case it might be a description like "The quick brown fox jumps over the Situation Object Structure Description" because "Situation Object Structure Description" is defined in SAP term this way.

We typically struggle, because object types like "class", "tables", and so on are defined as lower case in SAP term.

huber-nicolas · 2022-05-12T16:06:45Z

file-formats/sito/sito-v1.json

+              "sitnobjevtisreusable": {
+                "title": "Object Reusable",
+                "description": "Reusability Flag",
+                "type": "string",


is "type": "boolean" sufficient?

huber-nicolas · 2022-05-12T16:08:11Z

file-formats/sito/sito-v1.json

+                "title": "Object Reusable",
+                "description": "Reusability Flag",
+                "type": "string",
+                "enum": [


is "type": "boolean" sufficient?

huber-nicolas · 2022-05-12T16:08:50Z

file-formats/sito/sito-v1.json

+                "title": "Object Reusable",
+                "description": "Reusability Flag",
+                "type": "string",
+                "enum": [


is "type": "boolean" sufficient?

huber-nicolas · 2022-05-12T16:09:34Z

file-formats/sito/sito-v1.json

+                "description": "Reusability Flag",
+                "type": "string",
+                "enum": [
+                  "yes",


is "type": "boolean" sufficient?

huber-nicolas · 2022-05-12T16:09:54Z

file-formats/sito/sito-v1.json

+              "sitnobjvhsrvcisreusable": {
+                "title": "Object Reusable",
+                "description": "Reusability Flag",
+                "type": "string",


is "type": "boolean" sufficient?

schneidermic0

@tobiasmuenchsap Thank you for your contribution.

Before I go through details, I would like you to ask to change following in this pull request:

Would you also upload your interface (starting with Z) and (if possible) an example object. In SAP-internal systems, there is currently still a program you can use for this step to generate the whole folder as described in our how-to. This would also help that we can do the most part of the review in the ABAP interface itself.
I suggest to go over the field names (see one example in my comments) to get human-readable names. I would use underscores to split the name and also remove the IMHO unnecessary prefixes like sitnobj.

schneidermic0 · 2022-05-13T07:47:28Z

file-formats/sito/sito-v1.json

+                  "description": "SIT2_OBJ_STR_SK",
+                  "type": "object",
+                  "properties": {
+                    "sitnobjectfieldorder": {


Just one example, how I would rename the fields. In the ABAP interface it would be renamed to field_order instead of sitnobjectfieldorder:

Suggested change

"sitnobjectfieldorder": {

"fieldOrder": {

initial commit

abaplint · 2022-05-17T13:28:00Z

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

+
+    "! <p class="shorttext">List of SIT2_OBJ_STRUC_T</p>
+    "! List of SIT2_OBJ_STRUC_T
+    ty_sit2_obj_struc_t_list TYPE STANDARD TABLE OF ty_sit2_obj_struc_t WITH EMPTY KEY,


Downport EMPTY KEY

Suggest following fix,

Suggested change

ty_sit2_obj_struc_t_list TYPE STANDARD TABLE OF ty_sit2_obj_struc_t WITH EMPTY KEY,

ty_sit2_obj_struc_t_list TYPE STANDARD TABLE OF ty_sit2_obj_struc_t WITH DEFAULT KEY,

schneidermic0

Thanks for the update. The example and the ABAP type are helpful.
I have added a bunch of comments (see below).

However, here are some general remarks:

As mentioned before, I think the fields need to be renamed. It is really hard to understand which information needs to be specified and will be hard for users to understand the content of the JSON.
It would be helpful at a lot of places to use enum values instead of native values like "01" or "CB" (see https://github.com/SAP/abap-file-formats/blob/main/docs/json.md#enum-values)
Would you please fix the errors raised by abaplint?
I guess we can remove the translations since we want to specify descriptions only in the originalLanguage. As of now, translations shall be handled in dedicated files (tbd; see Handling translations #106).

schneidermic0 · 2022-05-17T19:08:47Z

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

@@ -0,0 +1,349 @@
+INTERFACE if_aff_sito_v1


Suggested change

INTERFACE if_aff_sito_v1

INTERFACE zif_aff_sito_v1

schneidermic0 · 2022-05-17T19:10:16Z

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

@@ -0,0 +1,349 @@
+INTERFACE if_aff_sito_v1
+  PUBLIC .


Suggested change

PUBLIC .

PUBLIC.

schneidermic0 · 2022-05-17T19:16:00Z

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

+    "! SIT2_OBJECT
+    BEGIN OF ty_sit2_object,
+      "! <p class="shorttext">Situation Object ID</p>
+      sitnobjectid          TYPE sit2_de_object_id,


Is this the object name? If so, we typically don't add them to the file formats. The object name is specified by the filename.

schneidermic0 · 2022-05-17T19:16:03Z

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

+      "! <p class="shorttext">ABAP Language Version</p>
+      abap_language_version TYPE abap_language_version,


Why do we need the ABAP language version twice. It is already stored in the header...

schneidermic0 · 2022-05-17T19:18:52Z

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

+      "! <p class="shorttext">ABAP Language Version</p>
+      abap_language_version TYPE abap_language_version,
+      "! <p class="shorttext">Object Extensible</p>
+      sitnobjisextensible   TYPE sit2_de_extensible,


As mentioned before, I suggest to rename the fields. E.g. something like extensible?

Suggested change

sitnobjisextensible TYPE sit2_de_extensible,

extensible TYPE sit2_de_extensible,

Furthermore, it would be great if the types in this interface would be self-contained.

schneidermic0 · 2022-05-17T19:30:44Z

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

+        "sitnobjeventcategory": "CL",
+        "sitnobjevtscope": "01",


schneidermic0 · 2022-05-17T19:31:25Z

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

+        "sitnactiontype": "02",
+        "sitnobjactscope": "01",


schneidermic0 · 2022-05-17T19:31:56Z

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

+        "sit2ObjActT": [
+          {
+            "language": "en",
+            "sitnactionname": "Upgrade Passengers",
+            "sitnactiondescription": "Upgrade economy passengers to business class"
+          },
+          {
+            "language": "de",
+            "sitnactionname": "Passagiere upgraden",
+            "sitnactiondescription": "Economy-Passagiere auf Business Class upgraden"
+          }
+        ],


Translations?

schneidermic0 · 2022-05-17T19:37:40Z

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

+        "sit2ObjCbPar": [
+          {
+            "sitncallbackparamname": "BOOKID"
+          },
+          {
+            "sitncallbackparamname": "CARRID"
+          },


If you rename the fields you can even think of something like this:

Suggested change

"sit2ObjCbPar": [

{

"sitncallbackparamname": "BOOKID"

},

{

"sitncallbackparamname": "CARRID"

},

"callbackParameters": [

{

"name": "BOOKID"

},

{

"name": "CARRID"

},

or even if you don't need additional information for the parameters.

Suggested change

"sit2ObjCbPar": [

{

"sitncallbackparamname": "BOOKID"

},

{

"sitncallbackparamname": "CARRID"

},

"parameters": [ "BOOKID", "CARRID"

schneidermic0 · 2022-05-17T19:38:40Z

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

+        "sitnobjvhsrvcpathtype": "01",
+        "sitnobjvhsrvcscope": "01",


schneidermic0 · 2022-07-21T14:30:02Z

As far as I can see this PR is replaced by #380

Create sito-v1.json

df13128

Initial pre-review version of SITO AFF

huber-nicolas reviewed May 12, 2022

View reviewed changes

schneidermic0 marked this pull request as draft May 13, 2022 07:27

schneidermic0 self-requested a review May 13, 2022 07:30

schneidermic0 requested changes May 13, 2022

View reviewed changes

tobiasmuench added 2 commits May 17, 2022 14:21

Create zif_aff_sito_v1.intf.abap

ce2bd33

initial commit

Create TM_SITO_TRANS_TEST_01.sito.json

01b1a0f

initial commit

abaplint bot reviewed May 17, 2022

View reviewed changes

schneidermic0 requested changes May 17, 2022

View reviewed changes

schneidermic0 mentioned this pull request May 25, 2022

2022.05b Iteration Plan #366

Closed

schneidermic0 mentioned this pull request Jun 22, 2022

2022.06 Iteration Plan #368

Closed

schneidermic0 changed the title ~~Create sito-v1.json~~ [SITO] File format for situation objects Jul 12, 2022

schneidermic0 closed this Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SITO] File format for situation objects #364

[SITO] File format for situation objects #364

tobiasmuenchsap commented May 12, 2022

cla-assistant bot commented May 12, 2022 •

edited

Loading

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

tobiasmuench May 23, 2022

schneidermic0 May 24, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

tobiasmuench May 23, 2022

schneidermic0 May 24, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

huber-nicolas May 12, 2022

schneidermic0 left a comment

schneidermic0 May 13, 2022

abaplint bot May 17, 2022

schneidermic0 left a comment •

edited

Loading

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 May 17, 2022

schneidermic0 commented Jul 21, 2022

	ty_sit2_obj_struc_t_list TYPE STANDARD TABLE OF ty_sit2_obj_struc_t WITH EMPTY KEY,
	ty_sit2_obj_struc_t_list TYPE STANDARD TABLE OF ty_sit2_obj_struc_t WITH DEFAULT KEY,

		"! <p class="shorttext">ABAP Language Version</p>
		abap_language_version TYPE abap_language_version,

	sitnobjisextensible TYPE sit2_de_extensible,
	extensible TYPE sit2_de_extensible,

[SITO] File format for situation objects #364

[SITO] File format for situation objects #364

Conversation

tobiasmuenchsap commented May 12, 2022

cla-assistant bot commented May 12, 2022 • edited Loading

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

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

schneidermic0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abaplint bot May 17, 2022

Choose a reason for hiding this comment

schneidermic0 left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneidermic0 commented Jul 21, 2022

cla-assistant bot commented May 12, 2022 •

edited

Loading

schneidermic0 left a comment •

edited

Loading