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

refactor INTF #85

Merged
merged 6 commits into from
May 12, 2021
Merged

refactor INTF #85

merged 6 commits into from
May 12, 2021

Conversation

larshp
Copy link
Collaborator

@larshp larshp commented May 10, 2021

With 16+ years in ABAP I'm a newbie, and I have trouble reading the source code, this refactors INTF and removes all INCLUDE STRUCTURE, hopefully making it easier for humans to read.

It does however, sacrifice some reuse and introduces a bit of duplication, but IMHO its okay

Note that the schema field is removed from the header! $schema is not a json schema property, its a thing implemented in some editors like vscode. Editors have mechanisms to identify the correct schema for a file, I don't think we want to persist a URL inside the files, it might change in the future.

single definitions refactored to abapdoc + ABAP, instead of ABAP + abapdoc + ABAP, to

  "! Language
  TYPES language TYPE c LENGTH 2.

from

  TYPES:
    "! Language
    language TYPE c LENGTH 2.

which also follows https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#do-not-chain-up-front-declarations

Copy link
Contributor

@huber-nicolas huber-nicolas left a comment

Choose a reason for hiding this comment

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

It looks good to me

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.

Nice pull request. Thanks!

I didn't really like the includes in the types as well. I just added them for the sake of reuse, but it is fine for me if we duplicate code here. :)

Note that the schema field is removed from the header! ...
I don't think we want to persist a URL inside the files, it might change in the future.

We have to specify in which content version the content was stored. How would you add this identification? For me the $schema made sense to use it also as content version identification.
But I think, you are right we don't need it necessarily in the ABAP type. This could be also passed by the corresponding implementations to distinguish the content version.

There are some issues with ABAP Doc comments. I made suggestions how to fix them.

file-formats/intf/zif_aff_intf_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intf/zif_aff_intf_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/intf/zif_aff_intf_v1.intf.abap Outdated Show resolved Hide resolved
@larshp
Copy link
Collaborator Author

larshp commented May 11, 2021

for versioning, I suggest deferring to #53 for possible solutions

suggestions committed

larshp and others added 3 commits May 11, 2021 11:13
Co-authored-by: Michael Schneider <micha.schneider@sap.com>
Co-authored-by: Michael Schneider <micha.schneider@sap.com>
Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
Co-authored-by: Michael Schneider <micha.schneider@sap.com>
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.
We can follow-up versioning separately.

@schneidermic0 schneidermic0 merged commit 05ef028 into main May 12, 2021
@schneidermic0 schneidermic0 deleted the hvam/intf1005 branch May 12, 2021 08:09
@larshp larshp mentioned this pull request May 16, 2021
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.

None yet

3 participants