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

Remove recommendation for ABAP Formatter #343

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sautermi0
Copy link
Member

@sautermi0 sautermi0 commented Oct 31, 2023

Until now, the guide recommends the usage of the ABAP Formatter. From my perspective, the ABAPCleaner is superior and is getting more and more features, so the guide should recommend this.

What do you think about this?

@sautermi0 sautermi0 added the Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules label Oct 31, 2023
@larshp
Copy link
Contributor

larshp commented Oct 31, 2023

A: use of the pretty printer can be enforced, https://docs.abapopenchecks.org/checks/06/
B: the pretty printer is standard tooling
C: there is no concept for handling changes
D: it totally misses the feedback loop
E: encouraging automatic rewriting of eg. a 5000 line class is more harmful than good IMHO

@sautermi0 sautermi0 marked this pull request as draft October 31, 2023 14:37
> why we do not give clear guidance for the type case of keywords.
>
> Read more in _Chapter 5: Formatting: Team Rules_ of [Robert C. Martin's _Clean Code_].
Always use the [ABAPCleaner](https://github.com/SAP/abap-cleaner), ideally with the default configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be the same as before: Always use your team's formatter settings. 😉

when name lengths etc. change; if you want to avoid this, consider dropping rules like
[Align assignments to the same object, but not to different ones](#align-assignments-to-the-same-object-but-not-to-different-ones).

Some of them might not be covered by automated code formatting tools, such as ABAP Formatter or ABAPCleaner, so manual effort to reformat statements might be necessary, but we recommend sticking with automated formatting as dar a s possible for the sake of consistency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some of them might not be covered by automated code formatting tools, such as ABAP Formatter or ABAPCleaner, so manual effort to reformat statements might be necessary, but we recommend sticking with automated formatting as dar a s possible for the sake of consistency.
Some of them might not be covered by automated code formatting tools, such as ABAP Formatter or ABAP Cleaner, so manual effort to reformat statements might be necessary, but we recommend sticking with automated formatting as much as possible for the sake of consistency.

@ConjuringCoffee
Copy link
Contributor

I generally support recommending the use of the ABAP Cleaner, but I think some more guidance and nuance is required.

  • In whatever way you do formatting, you should use your team's settings. This applies to both the ABAP Formatter and the ABAP Cleaner.
  • For the ABAP Cleaner, it should be encouraged to use a shared profile.
  • I'm missing some kind of guidance regarding the use of the ABAP Cleaner in files where the ABAP Cleaner would change a lot. Similar to what Lars said, it could be harmful to blindly do the clean-up on large, old files.

@larshp
Copy link
Contributor

larshp commented Nov 2, 2023

is it clean to dig into exactly how and which tooling to use? eg. it currently doesn't recommend to use ATC/Code Inspector, does SAP recommend using exceptions or not, and how to, use central ATC or not? should developers use SE24 or Eclipse or vscode or notepad to write clean code? It opens for an endless list of questions on the exact tooling. I've yet to understand what exactly SAP means by Clean Code, but is it tooling dependent?

I'd suggest to change the section to "Use common tooling inside the team", and then link to the tools sections for options and then each tool has to explain/recommend exactly how to use it.

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 clean-abap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants