[flutter_migrate] Reland apply, abandon, and status commands and add environment.dart #2723
Conversation
GaryQian
changed the title
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // @dart = 2.8 |
There was a problem hiding this comment.
We should definitely not be landing new non-NNBD code; see flutter/flutter#110016
There was a problem hiding this comment.
Yep, ill change this to be null safe, this package no longer has the overhead of being part of a non-null-safe test suite!
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // @dart = 2.8 |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // @dart = 2.8 |
| } | ||
| String commandOutput = result.stdout; | ||
| Map<String, Object?> mapping = <String, Object?>{}; | ||
| if (commandOutput.contains('{') && commandOutput.endsWith('}\n')) { |
There was a problem hiding this comment.
Why is the end check so specific here? It seems like it would be a lot less fragile to minor whitespace changes to either use a regex, or to trim() the output and check for } at the end.
| if (_mapping.containsKey(key) && | ||
| _mapping[key] != null && | ||
| _mapping[key] is String) { | ||
| return _mapping[key]! as String; |
There was a problem hiding this comment.
Why the null check and force-unwrap when the return type is nullable? It seems like this whole function could just be:
Object? value = _mapping[key];
return value is String? ? value as String? : null;
| if (_mapping.containsKey(key) && | ||
| _mapping[key] != null && | ||
| _mapping[key] is bool) { | ||
| return _mapping[key]! as bool; |
| expect(env.getString('invalid key') == null, true); | ||
| expect(env.getBool('invalid key') == null, true); | ||
|
|
||
| expect(env.getString('FlutterProject.directory') != null, true); |
There was a problem hiding this comment.
I don't see dummy process output setup here; is unit test actually calling Flutter?
Thats seems problematic, since it means this test could break at any time if Flutter changes anything. I would expect unit tests to run against mocked process output.
There was a problem hiding this comment.
Good catch, ill mock the flutter dependency part of this to prevent this potential breakage.
There was a problem hiding this comment.
Just a couple of nits from me. Does this ready for another pass from @stuartmorgan ?
| /// | ||
| /// Each key is the String URI-style description of a value in the Flutter tool | ||
| /// and is mapped to a String or boolean value. The mapping should align with the | ||
| /// JSON output of `flutter analuze --suggestions --machine`. |
There was a problem hiding this comment.
| /// JSON output of `flutter analuze --suggestions --machine`. | |
| /// JSON output of `flutter analyze --suggestions --machine`. |
| processManager = const LocalProcessManager(); | ||
| }); | ||
|
|
||
| setUpAll(() {}); |
GaryQian
added
the
autosubmit
GaryQian
force-pushed
the
migratecommands
branch
from
b1e4b0b
to
7a9491c
Compare
…environment.dart (flutter#2723) * Checkout commands files * fix licences and tests * Skip apply test on old flutter version * Formatting * Fix version booleans * Move flutter version test tcheck first * Working tests * Formatting * Env test * Docs and fix windows test * Null safe, extra test * Improve cleanliness checking * Regex and fake process manager * Formatting * Change env test contents * format * Nits
Re-lands the already-reviewed and landed subcommands
applyabandonandstatusAdds
environment.dartwhich queries the flutter_tools for details about the environment and other tools propertiesPreviously landed PRs: flutter/flutter#102785, flutter/flutter#102787, flutter/flutter#102789