From b3f63d38acfd8a29d25e75c8ad0020cb39904624 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Sat, 9 Oct 2021 04:03:03 -0700 Subject: [PATCH] Document why some lints aren't enabled and fix some minor issues. (#91527) --- analysis_options.yaml | 15 ++-- dev/devicelab/lib/framework/utils.dart | 12 +-- packages/flutter/lib/analysis_options.yaml | 5 ++ .../flutter_driver/lib/src/driver/driver.dart | 84 ++++++++++--------- .../lib/src/commands/packages.dart | 9 +- 5 files changed, 62 insertions(+), 63 deletions(-) create mode 100644 packages/flutter/lib/analysis_options.yaml diff --git a/analysis_options.yaml b/analysis_options.yaml index f34fbe346b..cd428eba05 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -27,13 +27,12 @@ analyzer: missing_required_param: warning # treat missing returns as a warning (not a hint) missing_return: warning - # allow having TODO/HACK comments in the code + # allow having TODO comments in the code todo: ignore - hack: ignore # allow self-reference to deprecated members (we do this because otherwise we have # to annotate every member in every test, assert, etc, when we deprecate something) deprecated_member_use_from_same_package: ignore - # TODO(https://github.com/flutter/flutter/issues/74381): + # TODO(ianh): https://github.com/flutter/flutter/issues/74381 # Clean up existing unnecessary imports, and remove line to ignore. unnecessary_import: ignore # Turned off until null-safe rollout is complete. @@ -92,12 +91,12 @@ linter: - avoid_unnecessary_containers - avoid_unused_constructor_parameters - avoid_void_async - # - avoid_web_libraries_in_flutter # not yet tested + # - avoid_web_libraries_in_flutter # we use web libraries in web-specific code, and our tests prevent us from using them elsewhere - await_only_futures - camel_case_extensions - camel_case_types - cancel_subscriptions - # - cascade_invocations # not yet tested + # - cascade_invocations # doesn't match the typical style of this repo - cast_nullable_to_non_nullable # - close_sinks # not reliable enough # - comment_references # blocked on https://github.com/dart-lang/linter/issues/1142 @@ -105,9 +104,9 @@ linter: - control_flow_in_finally # - curly_braces_in_flow_control_structures # not required by flutter style - deprecated_consistency - # - diagnostic_describe_all_properties # not yet tested + # - diagnostic_describe_all_properties # enabled only at the framework level (packages/flutter/lib) - directives_ordering - # - do_not_use_environment # we do this commonly + # - do_not_use_environment # there are appropriate times to use the environment, especially in our tests and build logic - empty_catches - empty_constructor_bodies - empty_statements @@ -125,7 +124,7 @@ linter: - library_private_types_in_public_api # - lines_longer_than_80_chars # not required by flutter style - list_remove_unrelated_type - # - literal_only_boolean_expressions # too many false positives: https://github.com/dart-lang/sdk/issues/34181 + # - literal_only_boolean_expressions # too many false positives: https://github.com/dart-lang/linter/issues/453 - missing_whitespace_between_adjacent_strings - no_adjacent_strings_in_list # - no_default_cases # too many false positives diff --git a/dev/devicelab/lib/framework/utils.dart b/dev/devicelab/lib/framework/utils.dart index ea2856d65e..a38a740141 100644 --- a/dev/devicelab/lib/framework/utils.dart +++ b/dev/devicelab/lib/framework/utils.dart @@ -21,22 +21,14 @@ String cwd = Directory.current.path; /// The local engine to use for [flutter] and [evalFlutter], if any. String? get localEngine { - // Use two distinct `defaultValue`s to determine whether a 'localEngine' - // declaration exists in the environment. - const bool isDefined = - String.fromEnvironment('localEngine', defaultValue: 'a') == - String.fromEnvironment('localEngine', defaultValue: 'b'); + const bool isDefined = bool.hasEnvironment('localEngine'); return isDefined ? const String.fromEnvironment('localEngine') : null; } /// The local engine source path to use if a local engine is used for [flutter] /// and [evalFlutter]. String? get localEngineSrcPath { - // Use two distinct `defaultValue`s to determine whether a - // 'localEngineSrcPath' declaration exists in the environment. - const bool isDefined = - String.fromEnvironment('localEngineSrcPath', defaultValue: 'a') == - String.fromEnvironment('localEngineSrcPath', defaultValue: 'b'); + const bool isDefined = bool.hasEnvironment('localEngineSrcPath'); return isDefined ? const String.fromEnvironment('localEngineSrcPath') : null; } diff --git a/packages/flutter/lib/analysis_options.yaml b/packages/flutter/lib/analysis_options.yaml new file mode 100644 index 0000000000..dbb6737b5c --- /dev/null +++ b/packages/flutter/lib/analysis_options.yaml @@ -0,0 +1,5 @@ +include: ../analysis_options.yaml + +linter: + rules: + # - diagnostic_describe_all_properties # blocked on https://github.com/dart-lang/sdk/issues/47418 diff --git a/packages/flutter_driver/lib/src/driver/driver.dart b/packages/flutter_driver/lib/src/driver/driver.dart index 3f746b9081..3f87dd994f 100644 --- a/packages/flutter_driver/lib/src/driver/driver.dart +++ b/packages/flutter_driver/lib/src/driver/driver.dart @@ -556,53 +556,55 @@ abstract class FlutterDriver { /// /// The image will be returned as a PNG. /// - /// HACK: There will be a 2-second artificial delay before screenshotting, - /// the delay here is to deal with a race between the driver script and - /// the raster thread (formerly known as the GPU thread). The issue is - /// that driver API synchronizes with the framework based on transient - /// callbacks, which are out of sync with the raster thread. - /// Here's the timeline of events in ASCII art: + /// **Warning:** This is not reliable. /// - /// ------------------------------------------------------------------- - /// Without this delay: - /// ------------------------------------------------------------------- - /// UI : <-- build --> - /// Raster: <-- rasterize --> - /// Gap : | random | - /// Driver: <-- screenshot --> + /// There is a two-second artificial delay before screenshotting. The delay + /// here is to deal with a race between the driver script and the raster + /// thread (formerly known as the GPU thread). The issue is that the driver + /// API synchronizes with the framework based on transient callbacks, which + /// are out of sync with the raster thread. /// - /// In the diagram above, the gap is the time between the last driver - /// action taken, such as a `tap()`, and the subsequent call to - /// `screenshot()`. The gap is random because it is determined by the - /// unpredictable network communication between the driver process and - /// the application. If this gap is too short, which it typically will - /// be, the screenshot is taken before the raster thread is done - /// rasterizing the frame, so the screenshot of the previous frame is - /// taken, which is wrong. + /// Here's the timeline of events in ASCII art: /// - /// ------------------------------------------------------------------- - /// With this delay, if we're lucky: - /// ------------------------------------------------------------------- - /// UI : <-- build --> - /// Raster: <-- rasterize --> - /// Gap : | 2 seconds or more | - /// Driver: <-- screenshot --> + /// --------------------------------------------------------------- + /// Without this delay: + /// --------------------------------------------------------------- + /// UI : <-- build --> + /// Raster: <-- rasterize --> + /// Gap : | random | + /// Driver: <-- screenshot --> /// - /// The two-second gap should be long enough for the raster thread to - /// finish rasterizing the frame, but not longer than necessary to keep - /// driver tests as fast a possible. + /// In the diagram above, the gap is the time between the last driver action + /// taken, such as a `tap()`, and the subsequent call to `screenshot()`. The + /// gap is random because it is determined by the unpredictable communication + /// channel between the driver process and the application. If this gap is too + /// short, which it typically will be, the screenshot is taken before the + /// raster thread is done rasterizing the frame, so the screenshot of the + /// previous frame is taken, which is not what is intended. /// - /// ------------------------------------------------------------------- - /// With this delay, if we're not lucky: - /// ------------------------------------------------------------------- - /// UI : <-- build --> - /// Raster: <-- rasterize randomly slow today --> - /// Gap : | 2 seconds or more | - /// Driver: <-- screenshot --> + /// --------------------------------------------------------------- + /// With this delay, if we're lucky: + /// --------------------------------------------------------------- + /// UI : <-- build --> + /// Raster: <-- rasterize --> + /// Gap : | 2 seconds or more | + /// Driver: <-- screenshot --> /// - /// In practice, sometimes the device gets really busy for a while and - /// even two seconds isn't enough, which means that this is still racy - /// and a source of flakes. + /// The two-second gap should be long enough for the raster thread to finish + /// rasterizing the frame, but not longer than necessary to keep driver tests + /// as fast a possible. + /// + /// --------------------------------------------------------------- + /// With this delay, if we're not lucky: + /// --------------------------------------------------------------- + /// UI : <-- build --> + /// Raster: <-- rasterize randomly slow today --> + /// Gap : | 2 seconds or more | + /// Driver: <-- screenshot --> + /// + /// In practice, sometimes the device gets really busy for a while and even + /// two seconds isn't enough, which means that this is still racy and a source + /// of flakes. Future> screenshot() async { throw UnimplementedError(); } diff --git a/packages/flutter_tools/lib/src/commands/packages.dart b/packages/flutter_tools/lib/src/commands/packages.dart index bb50782374..62588179c6 100644 --- a/packages/flutter_tools/lib/src/commands/packages.dart +++ b/packages/flutter_tools/lib/src/commands/packages.dart @@ -305,10 +305,11 @@ class PackagesInteractiveGetCommand extends FlutterCommand { List rest = argResults.rest; final bool isHelp = rest.contains('-h') || rest.contains('--help'); String target; - if (rest.length == 1 && (rest[0].contains('/') || rest[0].contains(r'\'))) { - // HACK: Supporting flutter specific behavior where you can pass a - // folder to the command. - target = findProjectRoot(globals.fs, rest[0]); + if (rest.length == 1 && (rest.single.contains('/') || rest.single.contains(r'\'))) { + // For historical reasons, if there is one argument to the command and it contains + // a multiple-component path (i.e. contains a slash) then we use that to determine + // to which project we're applying the command. + target = findProjectRoot(globals.fs, rest.single); rest = []; } else { target = findProjectRoot(globals.fs);