Skip to content

add dartfmt support #202

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 3 commits into from
Apr 23, 2015
Merged

add dartfmt support #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a7dc8c
add dartfmt support
devoncarew Apr 22, 2015
d7c997a
review comments
devoncarew Apr 23, 2015
0a799e0
fix windows build
devoncarew Apr 23, 2015
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
221 changes: 145 additions & 76 deletions lib/grinder_sdk.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,53 @@ Directory getSdkDir([List<String> cliArgs]) => cli_util.getSdkDir(cliArgs);

File get dartVM => joinFile(sdkDir, ['bin', _sdkBin('dart')]);

/// Utility tasks for for getting information about the Dart VM and for running
/// Dart applications.
class Dart {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this class be called Dart or DartSdk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DartVM?

Copy link
Contributor

Choose a reason for hiding this comment

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

I chose Dart since it wraps the dart executable, similar to how Pub wraps pub and Dart2js wraps dart2js. However we do have Analyzer for dartanalyzer, so we could do Dart2js -> Compiler and Dart -> Vm, but I kind of think the Dart prefix is nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it might be interesting to namespace all the sdk stuff, so:

class Sdk {

  /// Defaults the path to the current SDK.
  Sdk([String path]);

  static Directory get dir;

  final Vm vm;
  final Compiler compiler;
  final Analyzer analyzer;
  final Formatter formatter;
}

final Sdk sdk = new Sdk();
print('Sdk dir: ${sdk.dir}');
sdk.vm.run(...);
sdk.compiler.compile(...);
sdk.analyzer.analyze(...);
sdk.formatter.format(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this as Dart, and removed the location static field. That would apply to a wrapper around the dartsdk, but not one around the dartvm.

/// Run a dart [script] using [run_lib.run].
///
/// Returns the stdout.
static String run(String script,
{List<String> arguments : const [], bool quiet: false,
String packageRoot, String workingDirectory, int vmNewGenHeapMB,
int vmOldGenHeapMB}) {
List<String> args = [];

if (packageRoot != null) {
args.add('--package-root=${packageRoot}');
}

if (vmNewGenHeapMB != null) {
args.add('--new_gen_heap_size=${vmNewGenHeapMB}');
}

if (vmOldGenHeapMB != null) {
args.add('--old_gen_heap_size=${vmOldGenHeapMB}');
}

args.add(script);
args.addAll(arguments);

return run_lib.run(_sdkBin('dart'), arguments: args, quiet: quiet,
workingDirectory: workingDirectory);
}

static String version({bool quiet: false}) {
// TODO: We may want to run `dart --version` in order to know the version
// of the SDK that grinder has located.
//run_lib.run(_sdkBin('dart'), arguments: ['--version'], quiet: quiet);
// The stdout does not have a stable documented format, so use the provided
// metadata instead.
return Platform.version.substring(0, Platform.version.indexOf(' '));
}
}

//class DartSdk {
// /// Return the path to the current Dart SDK. This will return `null` if we are
// /// unable to locate the Dart SDK.
// static Directory get location => sdkDir;
//}

/**
* Utility tasks for executing pub commands.
*/
Expand Down Expand Up @@ -146,6 +193,104 @@ class Pub {
}
}

/**
* Utility tasks for invoking dart2js.
*/
class Dart2js {
/**
* Invoke a dart2js compile with the given [sourceFile] as input.
*/
static void compile(File sourceFile,
{Directory outDir, bool minify: false, bool csp: false}) {
if (outDir == null) outDir = sourceFile.parent;
File outFile = joinFile(outDir, ["${fileName(sourceFile)}.js"]);

if (!outDir.existsSync()) outDir.createSync(recursive: true);

List args = [];
if (minify) args.add('--minify');
if (csp) args.add('--csp');
args.add('-o${outFile.path}');
args.add(sourceFile.path);

run_lib.run(_sdkBin('dart2js'), arguments: args);
}

/**
* Invoke a dart2js compile with the given [sourceFile] as input.
*/
static Future compileAsync(File sourceFile,
{Directory outDir, bool minify: false, bool csp: false}) {
if (outDir == null) outDir = sourceFile.parent;
File outFile = joinFile(outDir, ["${fileName(sourceFile)}.js"]);

if (!outDir.existsSync()) outDir.createSync(recursive: true);

List args = [];
if (minify) args.add('--minify');
if (csp) args.add('--csp');
args.add('-o${outFile.path}');
args.add(sourceFile.path);

return run_lib.runAsync(_sdkBin('dart2js'), arguments: args)
.then((_) => null);
}

static String version({bool quiet: false}) =>
_AppVersion.parse(_run('--version', quiet: quiet)).version;

static String _run(String command, {bool quiet: false}) =>
run_lib.run(_sdkBin('dart2js'), quiet: quiet, arguments: [command]);
}

/**
* Utility tasks for invoking the analyzer.
*/
class Analyzer {
/// Analyze a single [File] or path ([String]).
static void analyze(fileOrPath,
{Directory packageRoot, bool fatalWarnings: false}) {
analyzeFiles([fileOrPath], packageRoot: packageRoot,
fatalWarnings: fatalWarnings);
}

/// Analyze one or more [File]s or paths ([String]).
static void analyzeFiles(List files,
{Directory packageRoot, bool fatalWarnings: false}) {
List args = [];
if (packageRoot != null) args.add('--package-root=${packageRoot.path}');
if (fatalWarnings) args.add('--fatal-warnings');
args.addAll(files.map((f) => f is File ? f.path : f));

run_lib.run(_sdkBin('dartanalyzer'), arguments: args);
}

static String version({bool quiet: false}) => _AppVersion.parse(run_lib.run(
_sdkBin('dartanalyzer'), quiet: quiet, arguments: ['--version'])).version;
}

/// Utility class for invoking `dartfmt` from the SDK. This wrapper requires
/// the `dartfmt` from SDK 1.9 and greater.
class DartFmt {
/// Run the `dartfmt` command with the `--overwrite` option. Format any files
/// in place.
static void format(fileOrPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd be better off moving everything over to constructors and instance methods instead of classes as containers for static methods. That would allow them to be:

  • configured via constructors
  • passed around as instances
  • mocked out for testing

So:

class Pub {
  Pub();
  get(...);
  //...
  PubGlobal get global => new PubGlobal();
}

final Pub pub = new Pub();

class Formatter {
  Formatter.sdk(...);
  Formatter([VersionConstraint constraint]);
  //...
}
pub.get(...);
pub.global.run(...);
sdk.formatter.format(...); // As suggested in another review comment.
new Formatter().format(...); // Uses pub.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of weird the Pub uses static members, but PubGlobal uses instance members. This would fix that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll comment there; I generally like the idea but am worried about the cost of more breaking changes.

if (fileOrPath is File) fileOrPath = fileOrPath.path;
_run('--overwrite', fileOrPath);
}

/// Run the `dartfmt` command with the `--dry-run` option. Return `true` if
/// any files would be changed by running the formatter.
static bool dryRun(fileOrPath) {
if (fileOrPath is File) fileOrPath = fileOrPath.path;
String results = _run('--dry-run', fileOrPath);
return results.trim().isNotEmpty;
}

static String _run(String option, String target, {bool quiet: false}) =>
run_lib.run(_sdkBin('dartfmt'), quiet: quiet, arguments: [option, target]);
}

/// Access the `pub global` commands.
class PubGlobal {
Set<String> _activatedPackages;
Expand Down Expand Up @@ -237,82 +382,6 @@ abstract class PubApp {
String toString() => packageName;
}

/**
* Utility tasks for invoking dart2js.
*/
class Dart2js {
/**
* Invoke a dart2js compile with the given [sourceFile] as input.
*/
static void compile(File sourceFile,
{Directory outDir, bool minify: false, bool csp: false}) {
if (outDir == null) outDir = sourceFile.parent;
File outFile = joinFile(outDir, ["${fileName(sourceFile)}.js"]);

if (!outDir.existsSync()) outDir.createSync(recursive: true);

List args = [];
if (minify) args.add('--minify');
if (csp) args.add('--csp');
args.add('-o${outFile.path}');
args.add(sourceFile.path);

run_lib.run(_sdkBin('dart2js'), arguments: args);
}

/**
* Invoke a dart2js compile with the given [sourceFile] as input.
*/
static Future compileAsync(File sourceFile,
{Directory outDir, bool minify: false, bool csp: false}) {
if (outDir == null) outDir = sourceFile.parent;
File outFile = joinFile(outDir, ["${fileName(sourceFile)}.js"]);

if (!outDir.existsSync()) outDir.createSync(recursive: true);

List args = [];
if (minify) args.add('--minify');
if (csp) args.add('--csp');
args.add('-o${outFile.path}');
args.add(sourceFile.path);

return run_lib.runAsync(_sdkBin('dart2js'), arguments: args)
.then((_) => null);
}

static String version({bool quiet: false}) =>
_AppVersion.parse(_run('--version', quiet: quiet)).version;

static String _run(String command, {bool quiet: false}) =>
run_lib.run(_sdkBin('dart2js'), quiet: quiet, arguments: [command]);
}

/**
* Utility tasks for invoking the analyzer.
*/
class Analyzer {
/// Analyze a single [File] or path ([String]).
static void analyze(fileOrPath,
{Directory packageRoot, bool fatalWarnings: false}) {
analyzeFiles([fileOrPath], packageRoot: packageRoot,
fatalWarnings: fatalWarnings);
}

/// Analyze one or more [File]s or paths ([String]).
static void analyzeFiles(List files,
{Directory packageRoot, bool fatalWarnings: false}) {
List args = [];
if (packageRoot != null) args.add('--package-root=${packageRoot.path}');
if (fatalWarnings) args.add('--fatal-warnings');
args.addAll(files.map((f) => f is File ? f.path : f));

run_lib.run(_sdkBin('dartanalyzer'), arguments: args);
}

static String version({bool quiet: false}) => _AppVersion.parse(run_lib.run(
_sdkBin('dartanalyzer'), quiet: quiet, arguments: ['--version'])).version;
}

String _sdkBin(String name) {
if (Platform.isWindows) {
return name == 'dart' ? 'dart.exe' : '${name}.bat';
Expand Down
62 changes: 0 additions & 62 deletions lib/grinder_tools.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,47 +50,6 @@ void defaultInit([GrinderContext context]) { }
/// artifacts in the `build/`.
void defaultClean([GrinderContext context]) => delete(BUILD_DIR);

/// Utility tasks for invoking dart.
class Dart {

/// Run a dart [script] using [run_lib.run].
///
/// Returns the stdout.
static String run(String script,
{List<String> arguments : const [], bool quiet: false,
String packageRoot, String workingDirectory, int vmNewGenHeapMB,
int vmOldGenHeapMB}) {
List<String> args = [];

if (packageRoot != null) {
args.add('--package-root=${packageRoot}');
}

if (vmNewGenHeapMB != null) {
args.add('--new_gen_heap_size=${vmNewGenHeapMB}');
}

if (vmOldGenHeapMB != null) {
args.add('--old_gen_heap_size=${vmOldGenHeapMB}');
}

args.add(script);
args.addAll(arguments);

return run_lib.run(_sdkBin('dart'), arguments: args, quiet: quiet,
workingDirectory: workingDirectory);
}

static String version({bool quiet: false}) {
// TODO: We may want to run `dart --version` in order to know the version
// of the SDK that grinder has located.
//run_lib.run(_sdkBin('dart'), arguments: ['--version'], quiet: quiet);
// The stdout does not have a stable documented format, so use the provided
// metadata instead.
return Platform.version.substring(0, Platform.version.indexOf(' '));
}
}

/**
* A utility class to run tests for your project.
*/
Expand Down Expand Up @@ -359,27 +318,6 @@ class ContentShell extends Chrome {
ContentShell() : super(_contentShellPath());
}

// TODO: Remove. This is only duplicated (from grinder_sdk.dart) temporarily.
bool _sdkOnPath;

String _sdkBin(String name) {
if (Platform.isWindows) {
return name == 'dart' ? 'dart.exe' : '${name}.bat';
} else if (Platform.isMacOS) {
// If `dart` is not visible, we should join the sdk path and `bin/$name`.
// This is only necessary in unusual circumstances, like when the script is
// run from the Editor on macos.
if (_sdkOnPath == null) {
_sdkOnPath = whichSync('dart', orElse: () => null) != null;
}

return _sdkOnPath ? name : '${sdkDir.path}/bin/${name}';
} else {
return name;
}
}
// TODO: remove

String _dartiumPath() {
final Map m = {
"linux": "chrome",
Expand Down
41 changes: 41 additions & 0 deletions test/grinder_sdk_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ main() {
expect(dartVM, isNotNull);
});

// test('DartSdk.location', () {
// expect(DartSdk.location, isNotNull);
// });

test('Dart.version', () {
expect(Dart.version(quiet: true), isNotEmpty);
});

test('dart2js version', () {
return mockContext.runZoned(() {
expect(Dart2js.version(), isNotNull);
Expand Down Expand Up @@ -93,4 +101,37 @@ main() {
expect(grinder.isGlobal, false);
});
});

group('grinder.sdk DartFmt', () {
FilePath temp;
File file;

setUp(() {
temp = FilePath.createSystemTemp();
file = temp.join('foo.dart').asFile;
file.writeAsStringSync('void main() {}');
});

tearDown(() {
temp.delete();
});

test('dryRun', () {
// TODO: Re-enable this test on windows when our bot has a 1.9.3 SDK on it.
if (Platform.isWindows) return;

bool wouldChange = DartFmt.dryRun(file);
expect(wouldChange, true);
});

test('format', () {
// TODO: Re-enable this test on windows when our bot has a 1.9.3 SDK on it.
if (Platform.isWindows) return;

String originalText = file.readAsStringSync();
DartFmt.format(file);
String newText = file.readAsStringSync();
expect(newText, isNot(equals(originalText)));
});
});
}
4 changes: 0 additions & 4 deletions test/grinder_tools_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@ import 'package:unittest/unittest.dart';

main() {
group('grinder.tools', () {
test('Dart.version', () {
expect(Dart.version(quiet: true), isNotEmpty);
});

test('Chrome.getBestInstalledChrome', () {
Chrome chrome = Chrome.getBestInstalledChrome();
// Expect that we can always locate a Chrome, even on the bots.
Expand Down