Skip to content

More fixes from enabling all of code metrics #4931

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

Merged
merged 1 commit into from
Dec 21, 2022
Merged

More fixes from enabling all of code metrics #4931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
96 changes: 93 additions & 3 deletions analysis_options.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
analyzer:
#strong-mode:
#implicit-casts: false # 367 issues
#implicit-dynamic: false # 169 issues
#language:
#strict-inference: true # 34 issues
#strict-raw-types: true # 103 issues
Expand Down Expand Up @@ -144,3 +141,96 @@ linter:
# - use_to_and_as_if_applicable # has false positives, so we prefer to catch this by code-review
- valid_regexps
# - void_checks # not yet tested

dart_code_metrics:
metrics:
cyclomatic-complexity: 20
number-of-parameters: 4
maximum-nesting-level: 5
metrics-exclude:
- test/**
rules:
# - arguments-ordering Too strict
- avoid-banned-imports
- avoid-cascade-after-if-null
- avoid-collection-methods-with-unrelated-types
- avoid-duplicate-exports
- avoid-dynamic
# - avoid-global-state TODO(jacobr): bunch of false positives around boolean flags.
# - avoid-ignoring-return-values
# - avoid-late-keyword
- avoid-missing-enum-constant-in-map
- avoid-nested-conditional-expressions
# - avoid-non-ascii-symbols TODO(jacobr): probably worth enabling.
# - avoid-non-null-assertion
# - avoid-passing-async-when-sync-expected TODO(jacobr): consider re-enabliing.
- avoid-redundant-async
- avoid-throw-in-catch-block

# - avoid-top-level-members-in-tests Doesn't seem to match our style.
- avoid-unnecessary-type-assertions
- avoid-unnecessary-type-casts
- avoid-unrelated-type-assertions
- avoid-unused-parameters
- ban-name
# - binary-expression-operand-order Some nice catches but too many false positives to enable.
- double-literal-format
# - format-comment TODO(jacobr): enable this one after fixing violations.
# TODO(jacobr): enable member-ordering. This catches a bunch of real style
# issues but would be alot of work to migrate.
# - member-ordering
# - newline-before-return TODO(jacobr): should be in the formatter if it was a rule to adopt.
- no-boolean-literal-compare
# - no-empty-block Too many false positives. However it does flag a bunch of code smells so possibly worth re-enabling.
# This one seems interesting but has too many false positives. Gave it a try.
# - no-equal-arguments:
# ignored-parameters:
# - height
# - width
# - double-literal-format
# - defaultSortColumn
# - left
# - right
# - top
# - bottom
# - bottomLeft
# - topLeft
# - enabledBorder
- no-equal-then-else
# - no-magic-number
# - no-object-declaration Too difficult to use along with avoiding dynamic particular for JSON decoding logic.
# - prefer-async-await TODO(jacobr): evaluate enabling.
- prefer-commenting-analyzer-ignores
# - prefer-conditional-expressions Too many false positives involving large conditional expressions.
# - prefer-correct-identifier-length Too many false positives with fine names like i and id.
# - prefer-correct-test-file-name TODO(jacobr): enable and fix violations.
- prefer-correct-type-name
# - prefer-enums-by-name Cannot able unless lint adds a special case for orElse
# - prefer-first TODO(jacobr): enable as a follow up PR.
# - prefer-immediate-return TODO(jacobr): enable as a follow up PR.
- prefer-iterable-of
- prefer-last
# - prefer-match-file-name
# TODO(jacobr): consider enabling or enabling to periodically audit.
# This one has a lot of false positives but is also quite nice.
# - prefer-moving-to-variable:
# allowed-duplicated-chains: 2
# - prefer-static-class
# TODO(jacobr): enable this one as a follow up CL. This one has a lot
# of violations but generally the style is aligned with DevTools.
# - prefer-trailing-comma
- tag-name
- always-remove-listener
# - avoid-border-all Micro-optimization to avoid a const constructor.
# - avoid-returning-widgets This one is nice but has a lot of false positives.
- avoid-shrink-wrap-in-lists
- avoid-unnecessary-setstate
- avoid-expanded-as-spacer
- avoid-wrapping-in-padding
- check-for-equals-in-render-object-setters
- consistent-update-render-object
# - prefer-const-border-radius TODO(jacobr): enable.
- prefer-correct-edge-insets-constructor
# - prefer-extracting-callbacks I'm not clear this is always a good idea. Seems like a workaround.
# - prefer-single-widget-per-file
- prefer-using-list-view
5 changes: 5 additions & 0 deletions packages/devtools_app/analysis_options.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,8 @@ analyzer:
# these files if you want to ensure code is not invalid except where
# expected.
- test/test_infra/test_data/syntax_highlighting/**

dart_code_metrics:
metrics-exclude:
- test/**
- test/test_infra/test_data/**
6 changes: 3 additions & 3 deletions packages/devtools_app/integration_test/test_infra/test_app_driver.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ abstract class _TestApp with IOMixin {
await started;
}

Future<int> killGracefully() async {
Future<int> killGracefully() {
_debugPrint('Sending SIGTERM to $runProcessId..');
Process.killPid(runProcessId);

Expand All @@ -119,7 +119,7 @@ abstract class _TestApp with IOMixin {
return killFuture;
}

Future<int> _killForcefully() async {
Future<int> _killForcefully() {
_debugPrint('Sending SIGKILL to $runProcessId..');
Process.killPid(runProcessId, ProcessSignal.sigkill);

Expand All @@ -141,7 +141,7 @@ abstract class _TestApp with IOMixin {
int? id,
Duration? timeout,
bool ignoreAppStopEvent = false,
}) async {
}) {
final response = Completer<Map<String, Object?>>();
late StreamSubscription<String> sub;
sub = stdoutController.stream.listen(
Expand Down
36 changes: 17 additions & 19 deletions packages/devtools_app/lib/src/app.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,21 +112,21 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
});

_isDarkThemeEnabled = preferences.darkModeTheme.value;
preferences.darkModeTheme.addListener(() {
addAutoDisposeListener(preferences.darkModeTheme, () {
Copy link
Contributor Author

@jacob314 jacob314 Dec 10, 2022

Choose a reason for hiding this comment

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

Fyi @polina-c this caught some cases where were adding listeners but never removing.

setState(() {
_isDarkThemeEnabled = preferences.darkModeTheme.value;
});
});

_vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value;
preferences.vmDeveloperModeEnabled.addListener(() {
addAutoDisposeListener(preferences.vmDeveloperModeEnabled, () {
setState(() {
_vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value;
});
});

_denseModeEnabled = preferences.denseModeEnabled.value;
preferences.denseModeEnabled.addListener(() {
addAutoDisposeListener(preferences.denseModeEnabled, () {
setState(() {
_denseModeEnabled = preferences.denseModeEnabled.value;
});
Expand Down Expand Up @@ -191,10 +191,10 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
}

Widget _buildTabbedPage(
BuildContext context,
BuildContext _,
Copy link
Contributor Author

@jacob314 jacob314 Dec 10, 2022

Choose a reason for hiding this comment

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

the lint about unused parameters was great although I wish it was a bit smarter about methods that were torn off in the same file.
I also wish it handled __ as well as _.

Copy link
Contributor

@incendial incendial Dec 12, 2022

Choose a reason for hiding this comment

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

Could you give more details, how smarter you want it to be?

I also wish it handled __ as well as _.

Looks like a bug to me, since the rule should ignore any amount of _. Could you share an example?

Copy link
Contributor Author

@jacob314 jacob314 Dec 13, 2022

Choose a reason for hiding this comment

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

_buildTabbedPage is private and the only use is within this file is where it torn off. Glad to hear any number of _ should be allowed.

Copy link
Contributor

@incendial incendial Dec 20, 2022

Choose a reason for hiding this comment

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

Do I understand correctly: you mean that for private methods used as tear off the rule should not check for unused parameters?

Copy link
Contributor Author

@jacob314 jacob314 Dec 20, 2022

Choose a reason for hiding this comment

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

correct as the unused parameters were only there to match the signature required to use it as a tearoff. Its similar to why you shouldn't complain about unused parameters for an @OverRide

Copy link
Contributor

@incendial incendial Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you! Added to backlog, will ship in the next release

String? page,
Map<String, String?> params,
DevToolsNavigationState? state,
DevToolsNavigationState? __,
) {
final vmServiceUri = params['uri'];

Expand Down Expand Up @@ -234,7 +234,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
.where((p) => !hide.contains(p.screenId))
.toList();
if (screens.isEmpty) return child ?? const SizedBox.shrink();
return _providedControllers(
return MultiProvider(
providers: _providedControllers(),
child: DevToolsScaffold(
embed: embed,
ideTheme: ideTheme,
Expand Down Expand Up @@ -277,8 +278,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
return DevToolsScaffold.withChild(
key: UniqueKey(),
ideTheme: ideTheme,
child: _providedControllers(
offline: true,
child: MultiProvider(
Copy link
Contributor Author

@jacob314 jacob314 Dec 10, 2022

Choose a reason for hiding this comment

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

@kenzieschmoll not sure if this is clearer.
Technically this doesn't solve the lint as the lint still gets mad about methods returning List but I think it solves the spirit and in some ways is a little clearer than the opaque helper.

Copy link
Member

@kenzieschmoll kenzieschmoll Dec 12, 2022

Choose a reason for hiding this comment

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

The only downside of pulling the MultiProvider out of the helper is that this creates some duplicated code where we have to manually specify MultiProvider wherever _providedControllers is used, but I do see how _providedControllers already sounds like it should return a List as is.

Why did child: _providedControllers trigger a lint to begin with?

Copy link
Contributor Author

@jacob314 jacob314 Dec 13, 2022

Choose a reason for hiding this comment

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

The lint is one to guide you towards creating widgets rather than creating functions that return widgets. It catches a lot of ugly code (particularly cases where the BuildContext was passed around) as well as some false positives like this.

Copy link
Contributor Author

@jacob314 jacob314 Dec 15, 2022

Choose a reason for hiding this comment

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

I wonder if the duplicate code is a little more idiomatic in that it is clear what we are doing is injecting providers.

Copy link
Contributor Author

@jacob314 jacob314 Dec 15, 2022

Choose a reason for hiding this comment

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

@incendial this was the case where I couldn't keep the code idiomatic and avoid the lint about functions returning widgets. Seems odd the lint ever fires for functions returning Lists of widgets. Notice that the lint still fires with my fix here as it is upset about the method called that returns a List of all providers for the class.

Copy link
Contributor

@incendial incendial Dec 20, 2022

Choose a reason for hiding this comment

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

I see, never seen this case before, tbh 😅. If I change the rule to ignore Providers, will this be enough? The more I look at this case, the more I don't understand how a possible "right way" without a method could look like.

Seems odd the lint ever fires for functions returning Lists of widgets

Very common case is when people create a private method returning List and then spread it in the widget's Column or Row.

Copy link
Contributor Author

@jacob314 jacob314 Dec 20, 2022

Choose a reason for hiding this comment

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

So far the only case that is really blocking me is the provider case. The Column and Row cases are interesting. I do buy that those cases are probably usually bugs.

Copy link
Contributor

@incendial incendial Dec 21, 2022

Choose a reason for hiding this comment

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

Thank you! Added to backlog, will ship in the next release

providers: _providedControllers(offline: true),
child: SnapshotScreenBody(snapshotArgs, _screens),
),
);
Expand All @@ -292,7 +293,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
ReportFeedbackButton(),
OpenAboutAction(),
],
child: _providedControllers(
child: MultiProvider(
providers: _providedControllers(),
child: const AppSizeBody(),
),
);
Expand All @@ -308,18 +310,13 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {

List<Screen> _visibleScreens() => _screens.where(shouldShowScreen).toList();

Widget _providedControllers({required Widget child, bool offline = false}) {
final _providers = widget.screens
List<Provider> _providedControllers({bool offline = false}) {
return widget.screens
.where(
(s) => s.providesController && (offline ? s.supportsOffline : true),
)
.map((s) => s.controllerProvider(routerDelegate))
.toList();

return MultiProvider(
providers: _providers,
child: child,
);
}

@override
Expand Down Expand Up @@ -456,7 +453,7 @@ class OpenSettingsAction extends StatelessWidget {
return DevToolsTooltip(
message: 'Settings',
child: InkWell(
onTap: () async {
onTap: () {
Copy link
Contributor Author

@jacob314 jacob314 Dec 10, 2022

Choose a reason for hiding this comment

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

here and elsewhere the lints caught a ton of cases where we were pretending we were doing useful async things when we weren't. Fyi @pq

Copy link
Contributor Author

@jacob314 jacob314 Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@kenzieschmoll let me know if these cases where we are doing async stuff in tap handlers is useful or noise.

Copy link
Contributor Author

@jacob314 jacob314 Dec 15, 2022

Choose a reason for hiding this comment

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

This one seems interesting but has too many false positives. Gave it a try including kitchen sink adding parameters to ignore. Generally it didn't seem to catch that many real issues but had tons of false positives. # - no-equal-arguments:

Do you mean that the amount of false positives was too much to even list in the rule config?
Yeah I was tossing tons of names in the false positives list in the rules config but not seeing any cases where the lint was adding value so I gave up.

avoid-returning-widgets This one is nice but has a lot of false positives. Would be great to figure out how to make it more tolerant of reasonable patterns where a separate widget would just make things ugly.

That's interesting. If I remember correctly, we cleaned it up from all the known false positives we had. Could you share the cases you consider false positive for this rule so I can handle them properly?
I tagged you in the comment where I tried to make this work idiomatically for a case where we needed a helper method that computed the list of all Providers to inject with a MultiProvider.

Copy link
Contributor

@incendial incendial Dec 20, 2022

Choose a reason for hiding this comment

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

Yeah I was tossing tons of names in the false positives list in the rules config but not seeing any cases where the lint was adding value so I gave up.

This rule initially was created for models mapping, when you can have like 2 String fields and then accidentally pass same value to both fields. Maybe it should be restricted somehow to not trigger on widgets, but I'm not sure.

I tagged you in the comment where I tried to make this work idiomatically for a case where we needed a helper method that computed the list of all Providers to inject with a MultiProvider.

Thank you, answered above.

Copy link
Contributor Author

@jacob314 jacob314 Dec 20, 2022

Choose a reason for hiding this comment

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

Maybe it should only apply to certain types of parameters. Seems most relevant for string fields where there is more of a sign of duplicate logic than for num fields or arbitrary class fields.

Copy link
Contributor

@incendial incendial Dec 21, 2022

Choose a reason for hiding this comment

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

Good point, I'll take some time to think

unawaited(
showDialog(
context: context,
Expand Down Expand Up @@ -505,7 +502,8 @@ class SettingsDialog extends StatelessWidget {
CheckboxSetting(
label: const Text('Enable analytics'),
listenable: analyticsController.analyticsEnabled,
toggle: analyticsController.toggleAnalyticsEnabled,
toggle: (enable) =>
unawaited(analyticsController.toggleAnalyticsEnabled(enable)),
gaItem: gac.analytics,
),
CheckboxSetting(
Expand Down Expand Up @@ -537,7 +535,7 @@ class CheckboxSetting extends StatelessWidget {

final ValueListenable<bool> listenable;

final Function(bool) toggle;
final void Function(bool) toggle;

final String gaItem;

Expand Down
2 changes: 1 addition & 1 deletion packages/devtools_app/lib/src/framework/about_dialog.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ class OpenAboutAction extends StatelessWidget {
return DevToolsTooltip(
message: 'About DevTools',
child: InkWell(
onTap: () async {
onTap: () {
unawaited(
showDialog(
context: context,
Expand Down
4 changes: 3 additions & 1 deletion packages/devtools_app/lib/src/framework/app_error_handling.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import '../shared/config_specific/logger/logger.dart';
/// application.
void setupErrorHandling(Future Function() appStartCallback) {
// First, run all our code in a new zone.
return runZonedGuarded(() async {
return runZonedGuarded(
// ignore: avoid-passing-async-when-sync-expected this ignore should be fixed.
() async {
WidgetsFlutterBinding.ensureInitialized();

final FlutterExceptionHandler? oldHandler = FlutterError.onError;
Expand Down
3 changes: 2 additions & 1 deletion packages/devtools_app/lib/src/framework/framework_core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ import '../shared/primitives/utils.dart';
import '../shared/scripts/script_manager.dart';
import '../shared/survey.dart';

typedef ErrorReporter = void Function(String title, dynamic error);
typedef ErrorReporter = void Function(String title, Object error);
Copy link
Contributor Author

@jacob314 jacob314 Dec 10, 2022

Choose a reason for hiding this comment

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

lints were good at catching a ton of uses of dynamic that were left in our code.


// TODO(jacobr): refactor this class to not use static members.
// ignore: avoid_classes_with_only_static_members
class FrameworkCore {
static void initGlobals() {
Expand Down
Loading