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

Recommend native ENUM over alternatives #293

Merged
merged 4 commits into from
Feb 20, 2023
Merged

Conversation

N2oB6n-SAP
Copy link
Member

This proposal recommends ENUM over enumeration patterns. ENUM has the obvious advantage of being native to the language with both syntax checks and type-safe compilation.

In the main styleguide ENUM is therefore recommended over constants interfaces. The enumeration subtopic has been adjusted as well to reflect that recommendation, yet present all object-oriented alternative patterns as before.

Copy link
Member

@sautermi0 sautermi0 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you :)

@sautermi0 sautermi0 added the Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules label Dec 12, 2022
@N2oB6n-SAP N2oB6n-SAP self-assigned this Jan 31, 2023
Comment on lines 83 to 84
DATA(severity_as_char) = CONV symsgty( /compatbl/message_severity=>info ). "yields 'I'
DATA(severity) = CONV /compatbl/message_severity=>type( 'X' ). "yields /compatbl/message_severity=>exit
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of comments in these examples violates the rule "Put comments before the statement they relate to"

Copy link
Member Author

Choose a reason for hiding this comment

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

While that's true and it has already been changed by a subsequent commit pushed to this PR I personally don't agree. Note that there is a discussion ongoing in #297.


To integrate native enumerations with legacy code that uses constants the `BASE TYPE` addition is available:
```ABAP
CLASS /compatbl/message_severity DEFINITION PUBLIC ABSTRACT FINAL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the styleguide works using the namespace /clean/ for its examples. Wouldn't it be better to continue using it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was to highlight that this is not the cleanest pattern, merely the cleanest you can get while still bridging the legacy gap. Hence /compatbl/. Whenever possible BASE TYPE should be left out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

(Note that the code section in the preceding paragraph which does not contain BASE TYPE uses /clean/ for that reason.)

As mentioned in the comments before there were some syntax errors and clean code violations.
@cla-assistant
Copy link

cla-assistant bot commented Feb 14, 2023

CLA assistant check
All committers have signed the CLA.

@ConjuringCoffee
Copy link
Contributor

I've tried the suggested compatibility approach and it sounds good to me.

About two years ago I tried to find a way to make ENUMs work for me, but I wasn't able to find one. I really wonder why that was... I can only assume I didn't know about the BASE TYPE keyword.

@cribuc
Copy link

cribuc commented Feb 20, 2023

One month has passed sine requesting for feedback, enough review votes (5+1), no veto, thus we merge the request

@cribuc cribuc merged commit 17fc2b9 into SAP:main Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants