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.

Sign up for GitHub

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

add additional lints to dart_flutter_team_lints #167

Merged
merged 6 commits into from
Sep 13, 2023
Merged

add additional lints to dart_flutter_team_lints #167

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a49e68f
add additional lints to dart_flutter_team_lints
devoncarew Sep 12, 2023
fc35292
mono_reo generate
devoncarew Sep 12, 2023
64192c8
dart fix
devoncarew Sep 12, 2023
e90352e
dartfmt
devoncarew Sep 12, 2023
05b02db
revert unintentional changes
devoncarew Sep 12, 2023
a3b5918
updates from review
devoncarew Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions .github/workflows/dart.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkgs/blast_repo/lib/src/top_level.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'package:blast_repo/src/tweaks/mono_repo_tweak.dart';
import 'package:git/git.dart';

import 'repo_tweak.dart';
import 'tweaks/auto_publish_tweak.dart';
import 'tweaks/dependabot_tweak.dart';
import 'tweaks/github_action_tweak.dart';
import 'tweaks/mono_repo_tweak.dart';
import 'tweaks/no_reponse_tweak.dart';
import 'utils.dart';

Expand Down
2 changes: 1 addition & 1 deletion pkgs/corpus/bin/deps.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void main(List<String> args) async {

var targetPackage = await pub.getPackageInfo(packageName);

final dateOneYearAgo = DateTime.now().subtract(Duration(days: 365));
final dateOneYearAgo = DateTime.now().subtract(const Duration(days: 365));

var limit = packageLimit == null ? null : int.parse(packageLimit);

Expand Down
6 changes: 3 additions & 3 deletions pkgs/corpus/lib/api.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class ApiUsage {
'packages': fromPackages.toJson(),
'libraries': fromLibraries.toJson(),
};
file.writeAsStringSync(JsonEncoder.withIndent(' ').convert(json));
file.writeAsStringSync(const JsonEncoder.withIndent(' ').convert(json));
}

/// Returns whether we found any references to the target package.
Expand All @@ -70,8 +70,8 @@ class ApiUsage {
}

static ApiUsage fromFile(PackageInfo packageInfo, File file) {
var json =
JsonDecoder().convert(file.readAsStringSync()) as Map<String, dynamic>;
var json = const JsonDecoder().convert(file.readAsStringSync())
as Map<String, dynamic>;
return ApiUsage(
packageInfo,
References.fromJson(json['packages'] as Map<String, dynamic>),
Expand Down
3 changes: 1 addition & 2 deletions pkgs/corpus/lib/report.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@

import 'dart:io';

import 'package:corpus/pub.dart';

import 'api.dart';
import 'pub.dart';
import 'utils.dart';

abstract class ReportTarget {
Expand Down
10 changes: 5 additions & 5 deletions pkgs/corpus/test/api_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ void main() {

setUp(() {
var json =
JsonDecoder().convert(_sampleUsageJson) as Map<String, dynamic>;
const JsonDecoder().convert(_sampleUsageJson) as Map<String, dynamic>;
sampleUsage = ApiUsage(
PackageInfo.from(
JsonDecoder().convert(_packageInfoJson) as Map<String, dynamic>),
PackageInfo.from(const JsonDecoder().convert(_packageInfoJson)
as Map<String, dynamic>),
References.fromJson(json['packages'] as Map<String, dynamic>),
References.fromJson(json['libraries'] as Map<String, dynamic>),
);
Expand All @@ -42,8 +42,8 @@ void main() {
sampleUsage.toFile(tempFile);

var result = ApiUsage.fromFile(
PackageInfo.from(
JsonDecoder().convert(_packageInfoJson) as Map<String, dynamic>),
PackageInfo.from(const JsonDecoder().convert(_packageInfoJson)
as Map<String, dynamic>),
tempFile,
);

Expand Down
4 changes: 2 additions & 2 deletions pkgs/corpus/test/data/dart_library_references.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ void main() {
map['three'] = 3;
print(map);

var local = Queue();
Queue.castFrom(local);
var local = Queue<Object>();
Queue.castFrom<Object, String>(local);
}
11 changes: 11 additions & 0 deletions pkgs/dart_flutter_team_lints/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
## 2.0.0

- Enable `strict-inference`.
- Added `avoid_unused_constructor_parameters`.
- Added `cancel_subscriptions`.
- Added `comment_references`.
- Added `prefer_const_constructors`.
- Added `prefer_relative_imports`.
- Added `test_types_in_equals`.
- Added `use_super_parameters`.

## 1.0.0

- Added `collection_methods_unrelated_type`.
Expand Down
8 changes: 8 additions & 0 deletions pkgs/dart_flutter_team_lints/lib/analysis_options.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ include: package:lints/recommended.yaml
analyzer:
language:
strict-casts: true
strict-inference: true

linter:
rules:
# consistency
- avoid_empty_else
- avoid_shadowing_type_parameters
- avoid_types_as_parameter_names
- avoid_unused_constructor_parameters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one could lead to forced breaking API changes if a constructor parameter stops being used but is still passed by callers.

I don't know if it would ever occur that a constructor argument starts being ignored and it's not already a breaking behavior change, but I'm a little paranoid about lints that could suggest breaking API changes because an author applying a quick fix is less likely to consider impact on usages than an author manually changing arguments...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, people will feel the need to address these issues, and will assume that the lint is telling them the right thing to do.

- camel_case_extensions
- combinators_ordering
- curly_braces_in_flow_control_structures
Expand All @@ -35,8 +37,10 @@ linter:
- lines_longer_than_80_chars
- omit_local_variable_types
- prefer_asserts_in_initializer_lists
- prefer_const_constructors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmundch - is there any risk of degrading IPL or any other negative impacts by making too many values const in widely used packages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion with this lint, but note that we do have it enabled in 22 of 29 sampled dart-lang packages, and flutter turns this lint on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it moves initialization of each of those values to be done eagerly during the IPL as you noted (as opposed to being lazy or on-demand at the time you need it.). The question comes down to how many constants we are talking about. If it's just a handful of them, it's fine, but if all the sudden this will increase the total number of constants by 2x, that's more concerning.

cc @rakudrama, although he is about to be on PTO for a while.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will degrade IPL but a few small constants will not make much difference. It is really hard to give advice here since it depends a lot on the number and qualities of the constants. A few moderate sized class constants would be fine, unless one of their fields were initialized with, say, a gargantuan map.

I'd like to make dart2js be smarter about the impact of larger constants on IPL (e.g. dart-lang/sdk#32943) but I don't think it will ever be zero.

I would say go ahead since constant widgets, being identical each frame, help the flutter framework more efficiently detect parts of the widget tree that are invariant. It can translate to better frame rate.

- prefer_generic_function_type_aliases
- prefer_is_empty
- prefer_relative_imports
- prefer_single_quotes
- prefer_typing_uninitialized_variables
- sort_pub_dependencies
Expand All @@ -47,18 +51,22 @@ linter:
- unnecessary_statements
- use_is_even_rather_than_modulo
- use_string_in_part_of_directives
- use_super_parameters

# correctness
- always_declare_return_types
- avoid_catching_errors
- avoid_dynamic_calls
- await_only_futures
- cancel_subscriptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this lint is reliable enough - there are false positives.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm - will remove

- collection_methods_unrelated_type
- comment_references
- dangling_library_doc_comments
- hash_and_equals
- implicit_call_tearoffs
- no_duplicate_case_values
- only_throw_errors
- test_types_in_equals
- throw_in_finally
- type_annotate_public_apis
- unawaited_futures
Expand Down
4 changes: 2 additions & 2 deletions pkgs/dart_flutter_team_lints/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
name: dart_flutter_team_lints
description: An analysis rule set used by the Dart and Flutter teams.
version: 1.0.0
version: 2.0.0
repository: https://github.com/dart-lang/ecosystem/tree/main/pkgs/dart_flutter_team_lints

environment:
sdk: '>=2.19.0 <3.0.0'
sdk: ^3.0.0

dependencies:
lints: ^2.0.0
Expand Down
3 changes: 1 addition & 2 deletions pkgs/firehose/lib/firehose.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
import 'dart:io';
import 'dart:math';

import 'package:firehose/src/repo.dart';

import 'src/github.dart';
import 'src/pub.dart';
import 'src/repo.dart';
import 'src/utils.dart';

const String _botSuffix = '[bot]';
Expand Down
3 changes: 2 additions & 1 deletion pkgs/firehose/lib/src/github.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
import 'dart:convert';
import 'dart:io';

import 'package:firehose/src/repo.dart';
import 'package:http/http.dart' as http;
import 'package:path/path.dart' as path;

import 'repo.dart';

// TODO:(devoncarew): Consider replacing some of this class with package:github.

class Github {
Expand Down
2 changes: 1 addition & 1 deletion pkgs/firehose/lib/src/health/health.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import 'dart:io';
import 'dart:math';

import 'package:collection/collection.dart';
import 'package:firehose/firehose.dart';

import '../../firehose.dart';
import '../github.dart';
import '../utils.dart';
import 'changelog.dart';
Expand Down
2 changes: 1 addition & 1 deletion pkgs/firehose/lib/src/repo.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@

import 'dart:io';

import 'package:firehose/src/changelog.dart';
import 'package:path/path.dart' as path;
import 'package:pubspec_parse/pubspec_parse.dart';
import 'package:yaml/yaml.dart' as yaml;

import 'changelog.dart';
import 'github.dart';

class Repository {
Expand Down
2 changes: 1 addition & 1 deletion pkgs/repo_manage/lib/issue_transfer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class TransferIssuesCommand extends ReportCommand {
}

print('Waiting a bit to allow Github to catch up...');
await Future<void>.delayed(Duration(seconds: 5));
await Future<void>.delayed(const Duration(seconds: 5));
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkgs/repo_manage/lib/weekly.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ class WeeklyCommand extends ReportCommand {
final now = DateTime.now();
final currentDay = now.weekday;
final thisWeek = now.subtract(Duration(days: currentDay - 1));
firstReportingDay = thisWeek.subtract(Duration(days: 7));
firstReportingDay = thisWeek.subtract(const Duration(days: 7));
}

lastReportingDay = firstReportingDay.add(Duration(days: 6));
lastReportingDay = firstReportingDay.add(const Duration(days: 6));
}

var repos = noteableRepos.map(RepositorySlug.full).toList();
Expand Down