dartfmt moved an `// ignore: lint` comment

[jamesderlin]

trafficstars

Running dartfmt on:

void main() {
  try {
    print('hi');
  } on Error catch (e) { // ignore: avoid_catching_errors
    print('bye');
  }
}

generates:

void main() {
  try {
    print('hi');
  } on Error catch (e) {
    // ignore: avoid_catching_errors
    print('bye');
  }
}

And now the // ignore: comment is no longer referring to the intended line.

I don't think that this is quite the same as https://github.com/dart-lang/dart_style/issues/546 since:

1. The line doesn't need to be wrapped (it's shorter than 80 columns, and other than the presence of the comment, there's no other reason to split the line).
2. I can't just add // ignore: to "the new lines if dartfmt splits something up".

I could move // ignore: to be above the targeted line, but I'd prefer not to since it then looks like the comment belongs in the try block. (That dartfmt indents the comment in that case doesn't help.)

(I am using dartfmt 1.2.8 on linux x64.)

[kevmoo]

cc @munificent

[munificent]

Interesting. So, in general, the behavior is deliberate. The formatter considers it unidiomatic to have a line comment on the same line as the beginning of a block and puts it on the next line instead. This behavior is useful for things like formatting generated code which may not have any useful newlines.

But when the comment is an ignore comment, that might not be the right behavior. If this comes up frequently enough, it might be worth giving special handling for comments that start with // ignore:. But I think the simpler solution is to just move the comment to the previous line. Yeah, it looks a little weird, but ignore comments are a little weird.

[jamesderlin]

I think it'd be reasonable to reformat // ignore: comments only if they're preceded by whitespace.

Another idea is to make comments in general match the indentation of the next non-comment line (if there isn't an intervening blank line).

[a14n]

Dup of #798

But when the comment is an ignore comment, that might not be the right behavior. If this comes up frequently enough, it might be worth giving special handling for comments that start with // ignore:. But I think the simpler solution is to just move the comment to the previous line. Yeah, it looks a little weird, but ignore comments are a little weird.

In some cases it could make comments harder to read. For instance:

if (true) {
  things;
} else { // ignore: some_lint_on_else
  things;
}

// here the ignore looks related to the _then_ block
if (true) {
  things;
  // ignore: some_lint_on_else
} else { 
  things;
}

[munificent]

In some cases it could make comments harder to read.

Fair, but I think those cases are pretty rare in practice and easily explained with a second human-oriented comment.

[jamesderlin]

This would happen for any code of the form:

try {
  doSomething();
} catch (e) { // ignore: avoid_catches_without_on_clauses
  log(e);
  rethrow;
}

which seems like a reasonable and not atypical pattern.

[munificent]

Good point. It's probably worth special casing // ignore: comments.