Skip to content

Commit

Permalink
Use the latest language version if we can't find a package. (#1573)
Browse files Browse the repository at this point in the history
Use the latest language version if we can't find a package.

In the new tall-style behavior where we look for a surrounding package
config to infer the language version of each file, we have to decide
what to do when that look-up process fails (either because there is no
package config, or it's malformed).

Initially, I had it error out on that file. That's consistent with the
library API where the formatter *requires* a language version before it
will do anything.

But it's not consistent with the language spec and our other tools. For
them, if there is no valid surrounding package config, the file should
be treated as the latest language version.

This PR does that.

It also fixes the catastrophic UX that the current code has which is
that when a file can't have its language version inferred... it is
silently skipped without telling the user anything. Oops. Now it just
formats the file as expected.
  • Loading branch information
munificent authored Oct 8, 2024
1 parent 9c17d3b commit 618883f
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 34 deletions.
16 changes: 6 additions & 10 deletions lib/src/io.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ Future<void> formatStdin(
if (languageVersion == null && path != null && cache != null) {
// We have a stdin-name, so look for a surrounding package config.
languageVersion = await cache.findLanguageVersion(File(path), path);

// If the lookup failed, don't try to parse the code.
if (languageVersion == null) return;
}

// If they didn't specify a version or a path, default to the latest.
// If they didn't specify a version or a path, or couldn't find a package
// surrounding the path, then default to the latest version.
languageVersion ??= DartFormatter.latestLanguageVersion;

// Determine the page width.
Expand Down Expand Up @@ -164,14 +162,12 @@ Future<bool> _processFile(
var languageVersion = options.languageVersion;
if (languageVersion == null && cache != null) {
languageVersion = await cache.findLanguageVersion(file, displayPath);

// If the lookup failed, don't try to parse the file.
if (languageVersion == null) return false;
} else {
// If they didn't specify a version or a path, default to the latest.
languageVersion ??= DartFormatter.latestLanguageVersion;
}

// If they didn't specify a version and we couldn't find a surrounding
// package, then default to the latest version.
languageVersion ??= DartFormatter.latestLanguageVersion;

// Determine the page width.
var pageWidth = options.pageWidth;
if (pageWidth == null && cache != null) {
Expand Down
22 changes: 10 additions & 12 deletions lib/src/testing/test_file_system.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,29 @@ final class TestFileSystem implements FileSystem {
}

@override
Future<bool> fileExists(FileSystemPath path) async =>
_files.containsKey(path.testPath);
Future<bool> fileExists(covariant TestFileSystemPath path) async =>
_files.containsKey(path._path);

@override
Future<FileSystemPath> join(FileSystemPath from, String to) async {
Future<FileSystemPath> join(
covariant TestFileSystemPath from, String to) async {
// If it's an absolute path, discard [from].
if (to.startsWith('|')) return TestFileSystemPath(to);
return TestFileSystemPath('${from.testPath}|$to');
return TestFileSystemPath('${from._path}|$to');
}

@override
Future<FileSystemPath?> parentDirectory(FileSystemPath path) async {
var parts = path.testPath.split('|');
Future<FileSystemPath?> parentDirectory(
covariant TestFileSystemPath path) async {
var parts = path._path.split('|');
if (parts.length == 1) return null;

return TestFileSystemPath(parts.sublist(0, parts.length - 1).join('|'));
}

@override
Future<String> readFile(FileSystemPath path) async {
if (_files[path.testPath] case var contents?) return contents;
Future<String> readFile(covariant TestFileSystemPath path) async {
if (_files[path._path] case var contents?) return contents;
throw Exception('No file at "$path".');
}
}
Expand All @@ -51,7 +53,3 @@ final class TestFileSystemPath implements FileSystemPath {
@override
String toString() => _path;
}

extension on FileSystemPath {
String get testPath => (this as TestFileSystemPath)._path;
}
43 changes: 31 additions & 12 deletions test/cli/language_version_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ extension type Meters(int value) {
}
''';

test('defaults to latest language version if omitted', () async {
test('uses latest language version if no surrounding package', () async {
await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create();

var process = await runFormatterOnDir();
// Enable the experiment to be sure that we're opting in to the new
// package config search.
var process = await runFormatterOnDir(['--enable-experiment=tall-style']);
await process.shouldExit(0);

await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate();
Expand Down Expand Up @@ -160,24 +162,26 @@ main() {
]).validate();
});

test('malformed', () async {
test('use the latest version if the package config is malformed', () async {
await d.dir('foo', [
d.dir('.dart_tool', [
d.file('package_config.json', 'this no good json is bad json'),
]),
d.file('main.dart', 'main() {}'),
d.file('main.dart', 'main() {var (a,b)=(1,2);}'),
]).create();

var path = p.join(d.sandbox, 'foo', 'main.dart');
// TODO(rnystrom): Remove experiment flag when it ships.
var process =
await runFormatter([path, '--enable-experiment=tall-style']);
var process = await runFormatterOnDir(['--enable-experiment=tall-style']);
await process.shouldExit(0);

expect(
await process.stderr.next,
allOf(startsWith('Could not read package configuration for'),
contains(p.join('foo', 'main.dart'))));
await process.shouldExit(65);
// Formats the file.
await d.dir('foo', [
d.file('main.dart', '''
main() {
var (a, b) = (1, 2);
}
''')
]).validate();
});
});

Expand Down Expand Up @@ -235,5 +239,20 @@ main() {
expect(await process.stdout.next, '}');
await process.shouldExit(0);
});

test('use latest language version if no surrounding package', () async {
await d.dir('foo', []).create();

var process = await runFormatter(
['--enable-experiment=tall-style', '--stdin-name=foo/main.dart']);
// Use some relatively recent syntax.
process.stdin.writeln('main() {var (a,b)=(1,2);}');
await process.stdin.close();

expect(await process.stdout.next, 'main() {');
expect(await process.stdout.next, ' var (a, b) = (1, 2);');
expect(await process.stdout.next, '}');
await process.shouldExit(0);
});
});
}

0 comments on commit 618883f

Please sign in to comment.