search

SAP / abap-cleaner

ABAP cleaner applies 95+ cleanup rules to ABAP code at a single keystroke
Apache License 2.0
453 stars 48 forks source link

Allow customization of `TODO` keyword #78

Open bnichell opened 1 year ago

bnichell commented 1 year ago

Several rules add TODO comments to the code. Teams could have conventions for TODO keywords. For this reason, I propose to allow customizing the TODO keyword.

If this is is being added, it could also make sense to allow skipping the addition of (ABAP cleaner) as an additional option.

Example:

Before:

    " TODO: variable is assigned but never used (ABAP cleaner)

After:

    " TODO_CLEANER: variable is assigned but never used
jmgrassau commented 1 year ago

Hi,

yes, I actually considered this already, but didn't follow up on it yet, because it would require introducing some central settings that apply to multiple rules (and we simply don't have a place for that yet).

I guess what you'd need is to allow editing the prefix (default "TODO:") and the suffix (default "(ABAP cleaner)"), and the actual message would be put in the middle?

Changing this (or using different settings within a team) would mean, however, that ABAP cleaner might not be able to remove comments that were created with different settings (e.g. if a variable was unused but is now used).

Kind regards, Jörg-Michael

bnichell commented 1 year ago

Hi Jörg-Michael,

I guess what you'd need is to allow editing the prefix (default "TODO:") and the suffix (default "(ABAP cleaner)"), and the actual message would be put in the middle?

Yes, that would be the two options I would see as well.

Changing this (or using different settings within a team) would mean, however, that ABAP cleaner might not be able to remove comments that were created with different settings (e.g. if a variable was unused but is now used).

That's correct, but I assume that would already be the case for other formatting rules, if the setup is not consistent within a team. Thus, I would see this issue as a team responsibility, to use a shared configuration and to ensure, that each member keeps the configuration up to date.

The only scenario I see, would be, that if unhandled comments are still present in the code base, at the time the configuration is being changed, it would not catch those and would add a duplicate comment. I am not sure, if there is an elegant way to avoid this.

Possibilities I see:

  1. Check for single-line comments containing the main message, e.g. "variable is assigned but never used" and replace them. Problem: if additional information has been added by the user in the same line, this would be lost.
  2. Check for single-line comments containing the main message, e.g. "variable is assigned but never used". If it exists, skip adding a new comment (even if the format is incorrect).
  3. Check for all known complete comments, e.g. all combinations of prefixes + message + suffixes (optionally with storing previously set pre-/suffixes) and replace them. Should be safe?!
  4. If a single-line comments containing the main message, e.g. "variable is assigned but never used" is found and does not match the current style: add additional suffix like "(comment does not match configuration; remove comment and rerun ABAP cleaner to update it)", if it is not present already. This could be made toggleable, with fallback to option 2.

My preference would be option 2 or 4 (with fallback to 2).

Cheers, Benedikt