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

Consider migrating categories to tags #57719

Closed
pq opened this issue Jun 11, 2018 · 10 comments
Closed

Consider migrating categories to tags #57719

pq opened this issue Jun 11, 2018 · 10 comments
Assignees
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Jun 11, 2018

In the current incarnation, lints can belong to a single category but moving forward it would be interesting to consider a tag approach. A few ideas for tags that come to mind:

  • FLUTTER
  • PACKAGE: MOCKITO
  • DEPRECATED
  • EXPENSIVE

But I'm just spit-balling!

/cc @srawlins

@pq pq added the type-enhancement A request for a change that isn't a bug label Jun 11, 2018
@srawlins
Copy link
Member

I like those. I also like, STYLE, PERFORMANCE, ERROR-PRONE, CODE BLOAT

@pq
Copy link
Member Author

pq commented Jun 12, 2018

👍

@srawlins qq:

ERROR-PRONE -- do you mean the lint is prone to error? (e.g., false positives?)

@srawlins
Copy link
Member

No, I mean like "Code that violates this lint is error-prone", like control_flow_in_finally, and no_adjacent_strings_in_list and unrelated_type_equality_checks.

@srawlins
Copy link
Member

srawlins commented Aug 6, 2018

I see now that the "categories" are actually Groups defined in the analyzer, with a few built-in values. It's ok having non-built-in values. But I'll have to update LintRule in the SDK to specify groups now instead of group.

LintGroup also uses groups for its compareTo:

int compareTo(LintRule other) {
var g = group.compareTo(other.group);
if (g != 0) {
return g;
}
return name.compareTo(other.name);
}

Strange. Having multiple groups per rule will break this notion.

@pq
Copy link
Member Author

pq commented Aug 7, 2018

Yeah. This wasn't entirely by design. The group stuff got pulled into analyzer in a bulk move to ease maintenance. It's probably not the best place for it to be if we're going to be changing it. As for compareTo, we can re-think it. There are probably better ways to establish natural order.

@pq
Copy link
Member Author

pq commented Sep 24, 2018

@jcollins-g just gave (well, is giving 😛) a demo of category support for dartdoc (dart-lang/dartdoc#1681) which is making me think we should seriously consider using it rather than creating our own new thing.

@srawlins
Copy link
Member

srawlins commented Jan 4, 2019

Hi @pq the category/tag concept doesn't seem very complex. Right now the Group class in analyzer is super small. What complexity would we avoid duplicating? In particular the Category and Categorization classes (from dartdoc's model.dart) I think have a lot of complexity that would make this harder to integrate than to just adjust the Group class.

@pq
Copy link
Member Author

pq commented Jan 4, 2019

Thanks for cycling back to this @srawlins! I'm going to pack-peddle (bad pun, ha) and say we should just do our own thing. Onward!

⚡️ 🚲 ⚡️

@srawlins
Copy link
Member

srawlins commented Jan 5, 2019

Excellent! Not high priority, but I'll change gears (🚲) after a few other items and look into this.

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Sep 22, 2022
@srawlins srawlins self-assigned this Jun 5, 2024
copybara-service bot referenced this issue Jun 7, 2024
Spelled out at https://github.com/dart-lang/linter/issues/1020

Fixes https://github.com/dart-lang/linter/issues/1020

This CL does not change the categories/groups that any rule has. We
can add/change categories in later CLs.

Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
Change-Id: Ief2856a6c472492bbad19fc95df172ef8d19fe7b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369861
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Sam Rawlins <srawlins@google.com>
@srawlins
Copy link
Member

srawlins commented Jun 7, 2024

Fixed with 6c6b104

@srawlins srawlins closed this as completed Jun 7, 2024
parlough referenced this issue in dart-lang/site-www Aug 7, 2024
Lints no longer are part of three specific groups, but now have
tags/groups (https://github.com/dart-lang/linter/issues/1020). This PR
removes the expectation of the three groups so that the lints keep
rendering. It also updates the source data to the latest version with no
groups.

As part of the work in
#4499, we'll surface the new
categories/tags later on :)

Closes dart-lang/sdk#56396
@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants