Change in dart/sdk[master]: make dart fix `apply` opt-in
Phil Quitslund (Gerrit)
Attention is currently required from: Devon Carew, Brian Wilkerson.
Phil Quitslund would like Devon Carew and Brian Wilkerson to review this change.
make dart fix `apply` opt-in
Change-Id: I4d9f53fdf7ceaf41de6455118adfa23afbc087eb
---
M pkg/dartdev/lib/src/commands/fix.dart
M pkg/dartdev/test/commands/fix_test.dart
2 files changed, 78 insertions(+), 29 deletions(-)
diff --git a/pkg/dartdev/lib/src/commands/fix.dart b/pkg/dartdev/lib/src/commands/fix.dart
index f93be4c..8d36708 100644
--- a/pkg/dartdev/lib/src/commands/fix.dart
+++ b/pkg/dartdev/lib/src/commands/fix.dart
@@ -19,17 +19,37 @@
static final NumberFormat _numberFormat = NumberFormat.decimalPattern();
+ // todo (pq): add go link once redirect is live (https://github.com/dart-lang/sdk/issues/44261)
+ static const String cmdDescription = '''Fix Dart source code.
+
+This tool looks for and fixes analysis issues that have associated automated
+fixes or issues that have associated package API migration information.
+
+To use the tool, run one of:
+- 'dart fix --dry-run' for a preview of the proposed changes for a project
+- 'dart fix --apply' to apply the changes''';
+
/// This command is hidden as it's currently experimental.
- FixCommand() : super(cmdName, 'Fix Dart source code.', hidden: true) {
+ FixCommand() : super(cmdName, cmdDescription, hidden: true) {
argParser.addFlag('dry-run',
abbr: 'n',
defaultsTo: false,
+ negatable: false,
help: 'Show which files would be modified but make no changes.');
- argParser.addFlag('compare-to-golden',
- defaultsTo: false,
- help:
- 'Compare the result of applying fixes to a golden file for testing.',
- hide: true);
+ argParser.addFlag(
+ 'apply',
+ defaultsTo: false,
+ negatable: false,
+ help: 'Apply the proposed changes.',
+ );
+ argParser.addFlag(
+ 'compare-to-golden',
+ defaultsTo: false,
+ negatable: false,
+ help:
+ 'Compare the result of applying fixes to a golden file for testing.',
+ hide: true,
+ );
}
@override
@@ -39,12 +59,18 @@
var dryRun = argResults['dry-run'];
var testMode = argResults['compare-to-golden'];
+ var apply = argResults['apply'];
var arguments = argResults.rest;
var argumentCount = arguments.length;
if (argumentCount > 1) {
usageException('Only one file or directory is expected.');
}
+ if (!apply && !dryRun && !testMode) {
+ printUsage();
+ return 1;
+ }
+
var dir =
argumentCount == 0 ? io.Directory.current : io.Directory(arguments[0]);
if (!dir.existsSync()) {
diff --git a/pkg/dartdev/test/commands/fix_test.dart b/pkg/dartdev/test/commands/fix_test.dart
index b0a1a70..d23dc68 100644
--- a/pkg/dartdev/test/commands/fix_test.dart
+++ b/pkg/dartdev/test/commands/fix_test.dart
@@ -5,6 +5,8 @@
import 'dart:io';
import 'package:cli_util/cli_logging.dart';
+import 'package:dartdev/src/commands/fix.dart';
+import 'package:path/path.dart' as path;
import 'package:test/test.dart';
import '../utils.dart';
@@ -13,6 +15,9 @@
group('fix', defineFix, timeout: longTimeout);
}
+/// Enable to run from local source (useful in development).
+const runFromSource = false;
+
void defineFix() {
TestProject p;
@@ -22,15 +27,35 @@
tearDown(() => p?.dispose());
+ ProcessResult runFix(List<String> args, {String workingDir}) {
+ if (runFromSource) {
+ var binary = path.join(Directory.current.path, 'bin', 'dartdev.dart');
+ return p.runSync(binary, ['fix', ...?args], workingDir: workingDir);
+ }
+ return p.runSync('fix', args, workingDir: workingDir);
+ }
+
test('none', () {
p = project(mainSrc: 'int get foo => 1;\n');
- var result = p.runSync('fix', [p.dirPath]);
+
+ var result = runFix([p.dirPath]);
+
+ expect(result.exitCode, 1);
+ expect(result.stderr, isEmpty);
+ expect(result.stdout, contains(FixCommand.cmdDescription));
+ });
+
+ test('--apply (none)', () {
+ p = project(mainSrc: 'int get foo => 1;\n');
+
+ var result = runFix(['--apply', p.dirPath]);
+
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(result.stdout, contains('Nothing to fix!'));
});
- test('no args', () {
+ test('--apply (no args)', () {
p = project(
mainSrc: '''
var x = "";
@@ -41,7 +66,7 @@
- prefer_single_quotes
''',
);
- var result = p.runSync('fix', [], workingDir: p.dirPath);
+ var result = runFix(['--apply'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(
@@ -53,10 +78,10 @@
]));
});
- test('dry-run', () {
+ test('--dry-run', () {
p = project(
mainSrc: '''
-class A {
+class A {
String a() => "";
}
@@ -67,11 +92,11 @@
analysisOptions: '''
linter:
rules:
- - annotate_overrides
+ - annotate_overrides
- prefer_single_quotes
''',
);
- var result = p.runSync('fix', ['--dry-run', '.'], workingDir: p.dirPath);
+ var result = runFix(['--dry-run', '.'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(
@@ -84,7 +109,7 @@
]));
});
- test('.', () {
+ test('--apply (.)', () {
p = project(
mainSrc: '''
var x = "";
@@ -95,7 +120,7 @@
- prefer_single_quotes
''',
);
- var result = p.runSync('fix', ['.'], workingDir: p.dirPath);
+ var result = runFix(['--apply', '.'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(
@@ -108,7 +133,7 @@
]));
});
- test('excludes', () {
+ test('--apply (excludes)', () {
p = project(
mainSrc: '''
var x = "";
@@ -122,13 +147,13 @@
- prefer_single_quotes
''',
);
- var result = p.runSync('fix', ['.'], workingDir: p.dirPath);
+ var result = runFix(['--apply', '.'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(result.stdout, contains('Nothing to fix!'));
});
- test('ignores', () {
+ test('--apply (ignores)', () {
p = project(
mainSrc: '''
// ignore: prefer_single_quotes
@@ -140,7 +165,7 @@
- prefer_single_quotes
''',
);
- var result = p.runSync('fix', ['.'], workingDir: p.dirPath);
+ var result = runFix(['--apply', '.'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
expect(result.stdout, contains('Nothing to fix!'));
@@ -150,7 +175,7 @@
test('different', () {
p = project(
mainSrc: '''
-class A {
+class A {
String a() => "";
}
@@ -161,12 +186,12 @@
analysisOptions: '''
linter:
rules:
- - annotate_overrides
+ - annotate_overrides
- prefer_single_quotes
''',
);
p.file('lib/main.dart.expect', '''
-class A {
+class A {
String a() => '';
}
@@ -174,8 +199,7 @@
String a() => '';
}
''');
- var result =
- p.runSync('fix', ['--compare-to-golden', '.'], workingDir: p.dirPath);
+ var result = runFix(['--compare-to-golden', '.'], workingDir: p.dirPath);
expect(result.exitCode, 1);
expect(result.stderr, isEmpty);
});
@@ -183,7 +207,7 @@
test('same', () {
p = project(
mainSrc: '''
-class A {
+class A {
String a() => "";
}
@@ -194,12 +218,12 @@
analysisOptions: '''
linter:
rules:
- - annotate_overrides
+ - annotate_overrides
- prefer_single_quotes
''',
);
p.file('lib/main.dart.expect', '''
-class A {
+class A {
String a() => '';
}
@@ -208,8 +232,7 @@
String a() => '';
}
''');
- var result =
- p.runSync('fix', ['--compare-to-golden', '.'], workingDir: p.dirPath);
+ var result = runFix(['--compare-to-golden', '.'], workingDir: p.dirPath);
expect(result.exitCode, 0);
expect(result.stderr, isEmpty);
});
To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.
Phil Quitslund (Gerrit)
Attention is currently required from: Devon Carew, Brian Wilkerson.
Devon Carew (Gerrit)
Attention is currently required from: Phil Quitslund, Brian Wilkerson.
Patch set 3:Code-Review +1
3 comments:
Patchset:
lgtm (one suggestion about the exit code for a bare 'dart fix')
File pkg/dartdev/lib/src/commands/fix.dart:
Patch Set #3, Line 48:
negatable+1
Patch Set #3, Line 71:
returnI think that the `dart fix` mode - running it bare - probably shouldn't exit with an error. `return 0` here?
To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.
Phil Quitslund (Gerrit)
Attention is currently required from: Brian Wilkerson.
2 comments:
Patchset:
Thanks!
File pkg/dartdev/lib/src/commands/fix.dart:
Patch Set #3, Line 71:
returnI think that the `dart fix` mode - running it bare - probably shouldn't exit with an error. […]
Sounds good!
To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.
Phil Quitslund (Gerrit)
Attention is currently required from: Brian Wilkerson.
Patch set 4:Commit-Queue +2
commit-bot@chromium.org (Gerrit)
Attention is currently required from: Brian Wilkerson.
CQ is trying the patch.
Note: The patchset #4 "exit code tweak" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://dart-review.googlesource.com/c/173200/4
Bot data: {"action": "start", "triggered_at": "2020-11-20T04:33:27.0Z", "revision": "02e4ff66b82aa33fc898fe31ac86681c0841a47e"}
commit-bot@chromium.org (Gerrit)
commi...@chromium.org submitted this change.
make dart fix `apply` opt-in
Change-Id: I4d9f53fdf7ceaf41de6455118adfa23afbc087eb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/173200
Commit-Queue: Phil Quitslund <pquit...@google.com>
Reviewed-by: Devon Carew <devon...@google.com>
---
M pkg/dartdev/lib/src/commands/fix.dart
M pkg/dartdev/test/commands/fix_test.dart
2 files changed, 78 insertions(+), 29 deletions(-)
diff --git a/pkg/dartdev/lib/src/commands/fix.dart b/pkg/dartdev/lib/src/commands/fix.dart
index f93be4c..59424b9 100644
+ return 0;
+ }
+
var dir =
argumentCount == 0 ? io.Directory.current : io.Directory(arguments[0]);
if (!dir.existsSync()) {
diff --git a/pkg/dartdev/test/commands/fix_test.dart b/pkg/dartdev/test/commands/fix_test.dart
index b0a1a70..d948b77 100644+ expect(result.exitCode, 0);To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.
Dart CI (Gerrit)
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/818b66544c2363209c2a33926128b6da4990b530
Brian Wilkerson (Gerrit)
Attention is currently required from: Phil Quitslund.
Patch set 5:Code-Review +1
4 comments:
File pkg/dartdev/lib/src/commands/fix.dart:
Personally I see no value in forcing the closing parenthesis to be on a separate line, but if that's the style you want then we should add a comma on line 38.
Patch Set #5, Line 69:
!apply && !dryRun && !testModeI missed this in my CL too, but we should check that exactly one of these options is set because they're mutually exclusive.
File pkg/dartdev/test/commands/fix_test.dart:
Patch Set #5, Line 19:
runFromSourceThank you!
Patch Set #5, Line 48:
--apply (none)Consider using groups to organize the tests: one group per combination of options.
If you do that, the parenthesized text will presumably become the name of the test and you can drop the parentheses.
To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.
Phil Quitslund (Gerrit)
5 comments:
File pkg/dartdev/lib/src/commands/fix.dart:
Patch Set #3, Line 48:
negatable+1
Ack
File pkg/dartdev/lib/src/commands/fix.dart:
Personally I see no value in forcing the closing parenthesis to be on a separate line, but if that's […]
Ack
Patch Set #5, Line 69:
!apply && !dryRun && !testMode
I missed this in my CL too, but we should check that exactly one of these options is set because the […]
Ack
File pkg/dartdev/test/commands/fix_test.dart:
Patch Set #5, Line 19:
runFromSourceThank you!
Ack
Patch Set #5, Line 48:
--apply (none)
Consider using groups to organize the tests: one group per combination of options. […]
Ack
To view, visit change 173200. To unsubscribe, or for help writing mail filters, visit settings.