sharezone-app icon indicating copy to clipboard operation
sharezone-app copied to clipboard

Comments with bad spacing should not be found/fixed via RegEx (false-positives)

Open Jonas-Sander opened this issue 2 years ago • 1 comments

In our Dart code we follow the convention of leaving spaces after the slahes of comments.
A comment with bad spacing is a comment that has no space after the slashes.
Good comment: // foo bar Bad comment: //foo bar

We have a

  1. CI step which searches for bad comments and fails if any are found
  2. the sz fix-comment-spacing command which fixes the bad comments by inserting spaces after slashes

When checking for / fixing comments with bad spacing there are false-positives as these bad comments are found via RegEx.

The current implementation in fix_comment_spacing_command.dart(link to old commit - might be updated already) uses a simple regex to find comments via their slashes:

/// Matches "//" and "///"
final _commentSlashesRegex = RegExp(r'\/{2,3}');

and then uses some simple heuristics to try and filter out stuff like urls etc.

If there are edge cases one can use add the following comment to a file: // ignore_for_file: no_spaces_after_comment_slashes which is a workaround for this issue.

Steps to Reproduce

  1. Edit a .dart file to include print('//should not get matched as this is not a real comment');
  2. Let the analyze CI step run or run sz fix-comment-spacing

Current broken behavior

The analyze step fails as the statement above is falsely interpreted as a comment with bad spacing. The sz fix-comment-spacing wrongly "fixes" the statement above by inserting a space behind the print statement.

Expected behavior

The analyze step should not fail. The sz fix-comment-spacing should not wrongly fix the statement.

More examples / edge-cases

// An empty comment with no spaces after is okay:
//
//

void main() {
 print('//the example from above');
}

// What about this:
///////////heey
// //Is this alright?

// An empty comment at the end of the file is probably also okay:
///

// https://www.this-has-slashes-in-url.com

See also the already existing test cases (link to old commit - might be already updated)

Additional context

Old Issue from GitLab: https://gitlab.com/codingbrain/sharezone/sharezone-app/-/issues/1455

Possible solution

The Dart code should not be checked by regex as in this case there will always be edge cases for which it's not simple to find out if it's a real comment or just something that looks like a comment inside a string.

A better solution would be to analyze the Dart code via its AST.
One could possibly create an analyzer plugin or create a new lint rule for the official Dart linter.

The last option should probably be preferred as it seems like less work and it will benefit all other users.
I opened a ticket in the dart-lang/linter repo: "New lint for leaving space between comment slashes (//) and content."

Jonas-Sander avatar Jan 25 '22 02:01 Jonas-Sander

https://github.com/invertase/dart_custom_lint

Jonas-Sander avatar Jul 03 '22 17:07 Jonas-Sander