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

Add new object type DRAS (abapGit#6950) #622

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

fvalves
Copy link

@fvalves fvalves commented Jun 18, 2024

Added directory for new object type DRAS (abapGit#6950)

Copy link

cla-assistant bot commented Jun 18, 2024

CLA assistant check
All committers have signed the CLA.

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, @fvalves.

I guess, similar changes should be done for DSFS, as well. Therefor, I wait a minute to not provide the same review. ;)

file-formats/dras/examples/z_aff_example_dras.dras.json Outdated Show resolved Hide resolved
file-formats/dras/README.md Outdated Show resolved Hide resolved
"! <p class="shorttext">Header</p>
"! Header
"! $required
header TYPE zif_aff_types_v1=>ty_header_60,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a short question? Is key-user-tooling supported for DRAS?

Copy link
Author

Choose a reason for hiding this comment

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

Not yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, If you do not use key-user tooling I suggest to use ty_header_60_cloud which only supports "standard" and "cloudDevelopmnet"

Suggested change
header TYPE zif_aff_types_v1=>ty_header_60,
header TYPE zif_aff_types_v1=>ty_header_60_cloud,

Copy link
Author

Choose a reason for hiding this comment

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

It will be supported in the future, though. Is it preferable to have it already prepared or should it be restricted now and changed back to ty_header_60 later?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to change it and change it back to ty_header_60 as soon as it is supported. As far as I can see this change should be compatible (in the future)

fvalves and others added 2 commits June 19, 2024 09:59
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.

If you don't support key user tooling (yet) I suggest to change the header.

Everything else looks good to me. Thanks for your updates. 👍

"! <p class="shorttext">Header</p>
"! Header
"! $required
header TYPE zif_aff_types_v1=>ty_header_60,
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, If you do not use key-user tooling I suggest to use ty_header_60_cloud which only supports "standard" and "cloudDevelopmnet"

Suggested change
header TYPE zif_aff_types_v1=>ty_header_60,
header TYPE zif_aff_types_v1=>ty_header_60_cloud,

@fvalves
Copy link
Author

fvalves commented Jun 19, 2024

@schneidermic0 sorry, I miscommunicated. There aren't any key-user tools yet, but they are supported.

@albertmink albertmink added the new-object This is a new object type added to AFF label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-object This is a new object type added to AFF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants