Skip to content

Conversation

@BeckerWdf
Copy link
Contributor

@BeckerWdf BeckerWdf commented May 28, 2025

Source type can also be "External Entity"

@BeckerWdf BeckerWdf force-pushed the ddls branch 2 times, most recently from 2c3b7ac to d749248 Compare May 28, 2025 14:17
@BeckerWdf BeckerWdf requested a review from Markus1812 May 28, 2025 14:24
@Markus1812
Copy link
Member

Adding a new value to an enum is technically incompatible, if no default is defined for the type (see here).

"! <p class="shorttext">Source Type</p>
"! Source type
"! $values {@link zif_aff_ddls_v1.data:co_source_type}
TYPES ty_source_type TYPE c LENGTH 1.

Is it possible to add a default value for ty_source_type?

@BeckerWdf
Copy link
Contributor Author

BeckerWdf commented May 28, 2025

Adding a new value to an enum is technically incompatible.

Interesting. We will add more and more of these source types once we "invent" new types.

Is it possible to add a default value for ty_source_type?

We could add "initial" as a default value though. How should be proceed?

@schneidermic0
Copy link
Contributor

Interesting. We will add more and more of these source types once we "invent" new types.

Maybe, some background information, why we declared this as incompatible change. How shall the system behave, if a user would port a DDLS object with a source type that is unknown in the target system, what information shall be stored in this system. If this should be "initial" or "notDefined", this is fine if your object type can handle this. In basically most cases, it was fine to set it to the most common value.

We could add "initial" as a default value though. How should be proceed?

I think this would be fine. As mentioned above "notDefined" might be also a possible value. I have no strong opinion on this.

@BeckerWdf
Copy link
Contributor Author

If this should be "initial" or "notDefined", this is fine if your object type can handle this.

Yes that should be possible. The source type states which kind of statement is in the source code (DEFINE VIEW, EXTEND VIEW, ...) So if you import a DDLS with a source type that the system does not (yet) know because that source type / kind of statement was added in a later release the following would happen.
You could import that but could not activate it (because you would get syntax errors).

In basically most cases, it was fine to set it to the most common value.

There is no "most common value" for this attribute in this case.

@schneidermic0
Copy link
Contributor

Yes that should be possible. The source type states which kind of statement is in the source code (DEFINE VIEW, EXTEND VIEW, ...) So if you import a DDLS with a source type that the system does not (yet) know because that source type / kind of statement was added in a later release the following would happen.
You could import that but could not activate it (because you would get syntax errors).

Then I suggest to go for this. If the ABAP field is set to the default value, it won't be serialized in the JSON, since it is not a required field. I think this makes also sense, in your case 👍

@BeckerWdf
Copy link
Contributor Author

Then I suggest to go for this. If the ABAP field is set to the default value, it won't be serialized in the JSON, since it is not a required field. I think this makes also sense, in your case 👍

Currently it's mandatory

      "! <p class="shorttext">Source Type</p>
      "! Source type
      "! $required
      source_type    TYPE ty_source_type,

So do you propose to change it to no longer be mandatory?

@schneidermic0
Copy link
Contributor

Currently it's mandatory

Sorry, I somehow missed this. I think I wouldn't change the mandatory annotation, if this is not really beneficial. During serialization it would end up with "notDefined" (or similar) in the JSON representation.

@BeckerWdf
Copy link
Contributor Author

I added "unknown" as additional value for source type

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, @BeckerWdf. Looks good to me.

I will ask @Markus1812 to re-review this change, too.

@Markus1812
Copy link
Member

Thanks for the update and the new unknown constant. Can you please also set this as a default for the type?

"! <p class="shorttext">Source Type</p>
"! Source type
"! $values {@link zif_aff_ddls_v1.data:co_source_type}
TYPES ty_source_type TYPE c LENGTH 1.

Below $values, can you please add the following line:

"! $default {@link zif_aff_ddls_v1.data:co_source_type.unknown}

When generating a new schema, this should then also include the new default value.

@BeckerWdf
Copy link
Contributor Author

Thanks for the update and the new unknown constant. Can you please also set this as a default for the type?

"! <p class="shorttext">Source Type</p>
"! Source type
"! $values {@link zif_aff_ddls_v1.data:co_source_type}
TYPES ty_source_type TYPE c LENGTH 1.

Below $values, can you please add the following line:

"! $default {@link zif_aff_ddls_v1.data:co_source_type.unknown}

When generating a new schema, this should then also include the new default value.

done

"! <p class="shorttext">Source Origin</p>
"! Source origin
"! $values {@link zif_aff_ddls_v1.data:co_source_origin}
"! $default {@link zif_aff_ddls_v1.data:co_source_type.unknown}
Copy link
Contributor

@schneidermic0 schneidermic0 Jun 4, 2025

Choose a reason for hiding this comment

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

I guess you want to set the default for ty_source_type (not ty_source_origin), don't you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈

Source type can also be "External Entity".
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, now. :)

Copy link
Member

@Markus1812 Markus1812 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 update

@Markus1812 Markus1812 merged commit 158e208 into SAP:main Jun 4, 2025
10 checks passed
@BeckerWdf BeckerWdf deleted the ddls branch August 26, 2025 11:46
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.

3 participants