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

proposal: comment_all_ignores #4860

Closed
5 tasks done
Number-3434 opened this issue Jan 31, 2024 · 6 comments
Closed
5 tasks done

proposal: comment_all_ignores #4860

Number-3434 opened this issue Jan 31, 2024 · 6 comments
Assignees
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug

Comments

@Number-3434
Copy link

Number-3434 commented Jan 31, 2024

comment_all_ignores

(See notes on naming in Writing Lints.)

Description

A short description suitable for display in console output.

Missing a required ignore reason. Try adding a reason for this ignore after the rule name separated by a comma.

Details

A detailed description that provides context and motivation. Can be used directly in the rule's documentation.

From the Flutter Style Guide:

comment all // ignores

Sometimes, it is necessary to write code that the analyzer is unhappy with.

If you find yourself in this situation, consider how you got there. Is the analyzer actually correct but you don’t want to admit it? Think about how you could refactor your code so that the analyzer is happy. If such a refactor would make the code better, do it. (It might be a lot of work…​ embrace the yak shave.)

If you are really really sure that you have no choice but to silence the analyzer, use // ignore: . The ignore directive should be on the same line as the analyzer warning.

If the ignore is temporary (e.g. a workaround for a bug in the compiler or analyzer, or a workaround for some known problem in Flutter that you cannot fix), then add a link to the relevant bug, as follows:

foo(); // ignore: lint_code, https://link.to.bug/goes/here

If the ignore directive is permanent, e.g. because one of our lints has some unavoidable false positives and in this case violating the lint is definitely better than all other options, then add a comment explaining why:

foo(); // ignore: lint_code, sadly there is no choice but to do
// this because we need to twiddle the quux and the bar is zorgle.

Kind

Enforces style advice.

Copied from the Flutter Style Guide.

Bad Examples

A few examples that demonstrate where this lint should fire.

foo(); // ignore: lint_code

// ignore: lint_code
foo();

// ignore_for_file: avoid_annotating_with_dynamic

void foo({dynamic e, Object f}) {  }

Good Examples

Here are a few examples that demonstrate a “good” adoption of this lint’s principle.

foo(); // ignore: lint_code, https://link.to.bug/goes/here

// ignore: lint_code, https://link.to.bug/goes/here
foo();

// ignore_for_file: avoid_annotating_with_dynamic, ignoring for clarity

void foo({dynamic e, Object f}) {  }

import 'package:flutter/material.dart';

extension CustomStateExtension<T extends StatefulWidget> on State<T> {
  /// Calls [setState] on the widget if it is currently [mounted].
  void setStateIfMounted(void Function() fn) {
    if (!mounted) return;

    // ignore: invalid_use_of_protected_member, necessary since we need to call setState
    setState(fn is Future<void> Function() ? () { fn(); } : fn);
  }
}

Discussion

Add any other motivation or useful context here.

Conflicts

This lint may be self-conflicting, i.e. the user may actually have to specify a comment on the ignore to ignore this rule.

Other

Once this lint gets approved, a further lint may be ignores_same_line, which will lint against multiple ignores on the same line.

Discussion checklist

  • This lint does not modify, complement, overlap, or conflict with existing rules.
  • No relevant issues (reported here, the SDK Tracker, or elsewhere).
  • No prior art (e.g., in other linters).
  • This proposal corresponds to Flutter Style Guide advice.
  • Motivated by real-world examples. The above example of setStateIfMounted is from a utility in one of my real-life private repositories.
@Number-3434 Number-3434 changed the title proposal: <comment_all_ignores> proposal: comment_all_ignores Jan 31, 2024
@srawlins
Copy link
Member

srawlins commented Feb 5, 2024

I'd love this rule. I miss it often, from other linters, haha.

@Number-3434
Copy link
Author

Number-3434 commented Feb 5, 2024

I'd love this rule. I miss it often, from other linters, haha.

Yeah, also you would have to add an ignore comment to ignore this rule :)

// ignore: comment_all_ignores, because it's getting annoying
// ignore: some_lint

@bwilkerson bwilkerson added the P3 A lower priority bug or feature request label Feb 5, 2024
@pq
Copy link
Member

pq commented Feb 5, 2024

@goderbauer: I'm guessing this'd be used at least for the flutter repo and maybe flutter recommended style?

@bwilkerson, @scheglov, @srawlins, @keertip, @kallentu: I wonder if this is something we'd consider for analyzer?

@goderbauer
Copy link
Contributor

I would love to enforce this for the flutter repo.

Not so sure about putting it in the flutter recommended style set, though, since it isn't really a lint specific to flutter code. Since it is about general dart code, if we think this is useful we should align across the dart universe and put it in the general recommended set.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
@srawlins srawlins self-assigned this Apr 22, 2024
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue May 10, 2024
In order to land this new rule, I add a new `IgnoredElement` subclass,
`IgnoredDiagnosticComment`, for any trailing comment text. This seems
like a pretty clean implementation. The text and offset are not used,
but it seems like it would be inconsistent to not offer them.

I also make a few IgnoreInfo fields private that are unused outside
the library.

Fixes dart-lang/linter#4860

Change-Id: Id6f949a317929dae8833e404d4c9f3c8e4f7bc90
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365488
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>
mockturtl added a commit to mockturtl/tidy that referenced this issue May 15, 2024
@srawlins
Copy link
Member

Implemented in dart-lang/sdk@607b3f5

@Number-3434
Copy link
Author

yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants