Skip to content
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.

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

feat: Support nested analysis options files #212

Merged
merged 23 commits into from
Jan 27, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9f2ca24
guard against project directories
TimWhiting May 22, 2023
c925ce8
format
TimWhiting May 22, 2023
deea5e6
search for pubspecs outside of analysis_options.yaml
TimWhiting May 22, 2023
33273a5
fix one test
TimWhiting May 23, 2023
c375d98
Update packages/custom_lint/lib/src/package_utils.dart
rrousselGit Jun 7, 2023
3d080e2
some cleanup, and add test
TimWhiting Jun 8, 2023
d9f5609
fix the logic so both pubspec.yaml & package_config must be present
TimWhiting Jun 8, 2023
5fd23c7
fix tests
TimWhiting Jun 9, 2023
c46b021
actually fix all tests
TimWhiting Jun 9, 2023
c6071d7
Update packages/custom_lint/lib/src/package_utils.dart
rrousselGit Jun 9, 2023
736c968
address review comments
TimWhiting Jun 10, 2023
6e171ff
Address comments
TimWhiting Jun 20, 2023
78c4a8c
update a couple more sdk constraints
TimWhiting Jun 20, 2023
0455aa9
fix tests
TimWhiting Jun 24, 2023
1e234b5
Merge remote-tracking branch 'upstream/main'
mrgnhnt96 Jan 5, 2024
39b10cc
revert unnecessary change
mrgnhnt96 Jan 5, 2024
fc7785a
feat: pass project directory to pubspec overrides and project package…
mrgnhnt96 Jan 5, 2024
ad57385
feat: remove package_config.json requirement when searching parent dirs
mrgnhnt96 Jan 5, 2024
7b5d15e
feat: get projects instead of directories with analysis options
mrgnhnt96 Jan 5, 2024
a4b4518
address PR comments
mrgnhnt96 Jan 9, 2024
bafad81
Merge branch 'main' into main
mrgnhnt96 Jan 9, 2024
859470e
Review
rrousselGit Jan 26, 2024
95de80d
Merge branch 'main' into main
rrousselGit Jan 27, 2024
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
51 changes: 51 additions & 0 deletions packages/custom_lint/lib/src/package_utils.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'dart:io';

import 'package:meta/meta.dart';
import 'package:package_config/package_config.dart';
import 'package:path/path.dart';
import 'package:pubspec_parse/pubspec_parse.dart';
Expand Down Expand Up @@ -155,8 +156,58 @@ Future<PackageConfig?> tryParsePackageConfig(Directory directory) async {
/// does not exists.
Future<PackageConfig> parsePackageConfig(Directory directory) async {
final packageConfigFile = directory.packageConfig;

return PackageConfig.parseBytes(
await packageConfigFile.readAsBytes(),
packageConfigFile.uri,
);
}

/// Finds the project directory associated with an analysis context root, or null if it is not found
///
/// This is a folder that contains both a `pubspec.yaml` and a `.dart_tool/package_config.json` file.
/// It is either alongside the analysis_options.yaml file, or in a parent directory.
Directory? tryFindProjectDirectory(
Directory directory, {
Directory? original,
}) {
try {
return findProjectDirectory(
directory,
original: original,
);
} on FileSystemException {
return null;
}
}

/// Finds the project directory associated with an analysis context root
///
/// This is a folder that contains a `pubspec.yaml` file.
/// It is either alongside the analysis_options.yaml file, or in a parent directory.
Directory findProjectDirectory(
Directory directory, {
Directory? original,
}) {
final pubspecFile = directory.pubspec.existsSync();
if (pubspecFile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
final pubspecFile = directory.pubspec.existsSync();
if (pubspecFile) {
if (directory.pubspec.existsSync()) {

return directory;
}

if (directory.parent.uri == directory.uri) {
throw FindProjectError._(original?.path ?? directory.path);
}

return findProjectDirectory(directory.parent, original: directory);
}

/// No pubspec.yaml file was found for a plugin.
@internal
class FindProjectError extends FileSystemException {
/// An error that represents the folder [path] where the search for the pubspec started.
FindProjectError._(String path)
: super('Failed to find dart project at $path:\n', path);

@override
String toString() => message;
}
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,6 @@ class CustomLintRequestFailure implements Exception {

@override
String toString() {
return 'A request throw the exception:$message\n$stackTrace';
return 'A request threw the exception:$message\n$stackTrace';
}
}
43 changes: 31 additions & 12 deletions packages/custom_lint/lib/src/workspace.dart
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,13 @@ class CustomLintWorkspace {

final contextRootsWithCustomLint = await Future.wait(
allContextRoots.map((contextRoot) async {
final pubspecFile = Directory(contextRoot.root.path).pubspec;
if (!pubspecFile.existsSync()) {
return null;
}
final projectDir = tryFindProjectDirectory(
Directory(contextRoot.root.path),
);
if (projectDir == null) return null;

final pubspec = await tryParsePubspec(projectDir);
if (pubspec == null) return null;

final optionFile = contextRoot.optionsFile;
if (optionFile == null) {
Expand Down Expand Up @@ -492,8 +495,10 @@ class CustomLintWorkspace {
final uniquePluginNames =
projects.expand((e) => e.plugins).map((e) => e.name).toSet();

final realProjects = projects.where((e) => e.isProjectRoot).toList();

return CustomLintWorkspace._(
projects,
realProjects,
contextRoots,
uniquePluginNames,
workingDirectory: workingDirectory,
Expand Down Expand Up @@ -857,7 +862,8 @@ class CustomLintPluginCheckerCache {
if (cached != null) return cached;

return _cache[directory] = Future(() async {
final pubspec = await parsePubspec(directory);
final pubspec = await tryParsePubspec(directory);
if (pubspec == null) return false;

// TODO test that dependency_overrides & dev_dependencies aren't checked.
return pubspec.dependencies.containsKey('custom_lint_builder');
Expand Down Expand Up @@ -968,6 +974,7 @@ class CustomLintProject {
required this.packageConfig,
required this.pubspec,
required this.pubspecOverrides,
required this.analysisDirectory,
});

/// Decode a [CustomLintProject] from a directory.
Expand All @@ -976,18 +983,18 @@ class CustomLintProject {
CustomLintPluginCheckerCache cache,
) async {
final directory = Directory(contextRoot.root);

final projectPubspec = await parsePubspec(directory)
final projectDirectory = findProjectDirectory(directory);
final projectPubspec = await parsePubspec(projectDirectory).catchError(
// ignore: avoid_types_on_closure_parameters
.catchError((Object err, StackTrace stack) {
(Object err, StackTrace stack) {
throw PubspecParseError._(
directory.path,
error: err,
errorStackTrace: stack,
);
});
final pubspecOverrides = await tryParsePubspecOverrides(directory);
final projectPackageConfig = await parsePackageConfig(directory)
final pubspecOverrides = await tryParsePubspecOverrides(projectDirectory);
final projectPackageConfig = await parsePackageConfig(projectDirectory)
// ignore: avoid_types_on_closure_parameters
.catchError((Object err, StackTrace stack) {
throw PackageConfigParseError._(
Expand Down Expand Up @@ -1027,7 +1034,8 @@ class CustomLintProject {

return CustomLintProject._(
plugins: plugins.whereNotNull().toList(),
directory: directory,
directory: projectDirectory,
analysisDirectory: directory,
packageConfig: projectPackageConfig,
pubspec: projectPubspec,
pubspecOverrides: pubspecOverrides,
Expand All @@ -1044,10 +1052,21 @@ class CustomLintProject {
final Map<String, Dependency>? pubspecOverrides;

/// The folder of the project being analyzed.
/// Generally, where the pubspec.yaml is located
final Directory directory;

/// The enabled plugins for this project.
final List<CustomLintPlugin> plugins;

/// Where the analysis options file is located
/// It could be null if the project doesn't have an analysis options file.
///
/// The analysis options file doesn't not have to be in [directory]
final Directory? analysisDirectory;

bool get isProjectRoot {
return analysisDirectory == directory;
}
}

class _PackageAndPubspec {
Expand Down
3 changes: 2 additions & 1 deletion packages/custom_lint/test/cli_process_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ Analyzing...
'dependency conflict',
() async {
// Create two packages with the same name but different paths
final workspace = await createSimpleWorkspace(['dep', 'dep']);
final workspace =
await createSimpleWorkspace(['dep', 'dep'], local: true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a trailling comma, then format :)

Suggested change
await createSimpleWorkspace(['dep', 'dep'], local: true);
await createSimpleWorkspace(['dep', 'dep'], local: true,);


final plugin = createPlugin(
parent: workspace,
Expand Down
92 changes: 80 additions & 12 deletions packages/custom_lint/test/src/workspace_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ void writeSimplePackageConfig(
Future<Directory> createSimpleWorkspace(
List<Object> projectEntry, {
bool withPackageConfig = true,
bool local = false,
}) async {
/// The number of time we've created a package with a given name.
final packageCount = <String, int>{};
Expand All @@ -249,7 +250,7 @@ Future<Directory> createSimpleWorkspace(
return folderName;
}

return createWorkspace(withPackageConfig: withPackageConfig, {
return createWorkspace(local: local, withPackageConfig: withPackageConfig, {
for (final projectEntry in projectEntry)
if (projectEntry is Pubspec)
getFolderName(projectEntry.name): projectEntry
Expand All @@ -275,8 +276,10 @@ Future<Directory> createSimpleWorkspace(
Future<Directory> createWorkspace(
Map<String, Pubspec> pubspecs, {
bool withPackageConfig = true,
bool withNestedAnalysisOptions = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unused

bool local = false,
}) async {
final dir = createTemporaryDirectory();
final dir = createTemporaryDirectory(local: local);

String packagePathOf(Dependency dependency, String name) {
switch (dependency) {
Expand Down Expand Up @@ -339,10 +342,16 @@ Future<Directory> createWorkspace(
return dir;
}

Directory createTemporaryDirectory() {
final dir = Directory.current //
.dir('.dart_tool')
.createTempSync('custom_lint_test');
Directory createTemporaryDirectory({bool local = false}) {
final Directory dir;
if (local) {
// The cli_process_test needs it to be local in order for the relative paths to match
dir =
Directory.current.dir('.dart_tool').createTempSync('custom_lint_test');
} else {
// Others need global directory in order to not pick up this project's package_config.json
dir = Directory.systemTemp.createTempSync('custom_lint_test');
}
addTearDown(() => dir.deleteSync(recursive: true));

// Watches process kill to delete the temporary directory.
Expand Down Expand Up @@ -2453,25 +2462,67 @@ dependency_overrides:
});

group('fromContextRoots', () {
test('throws MissingPubspecError if package does not contain a pubspec',
/// Shorthand for calling [CustomLintWorkspace.fromContextRoots] from
/// a list of path.
Future<CustomLintWorkspace> fromContextRootsFromPaths(
List<String> paths, {
required Directory workingDirectory,
}) {
return CustomLintWorkspace.fromContextRoots(
paths.map((path) => ContextRoot(path, [])).toList(),
workingDirectory: workingDirectory,
);
}

test(
'finds pubspecs above analysis options file if there exists one',
() async {
final workspace = await createSimpleWorkspace(['package']);

final analysisFile = File(
p.join(workspace.path, 'package', 'analysis_options.yaml'),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use .dir('package').analysisOptions

analysisFile.createSync();
analysisFile.writeAsStringSync(analysisOptionsWithCustomLintEnabled);
final nestedAnalysisFile = File(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

p.join(workspace.path, 'package', 'test', 'analysis_options.yaml'),
);
nestedAnalysisFile.createSync(recursive: true);
nestedAnalysisFile
.writeAsStringSync(analysisOptionsWithCustomLintEnabled);

final customLintWorkspace = await CustomLintWorkspace.fromPaths(
[p.join(workspace.path, 'package')],
workingDirectory: workspace,
);
// Expect one context root for the workspace and one for the test folder
expect(customLintWorkspace.contextRoots.length, equals(2));
},
);

test(
'throws PackageConfigParseError if package has a pubspec but no .dart_tool/package_config.json',
() async {
final workspace = await createSimpleWorkspace([]);
workspace.dir('package').createSync(recursive: true);
final workspace = await createSimpleWorkspace(['package']);
workspace.dir('package', '.dart_tool').deleteSync(recursive: true);

expect(
() => fromContextRootsFromPaths(
[p.join(workspace.path, 'package')],
workingDirectory: workspace,
),
throwsA(isA<PubspecParseError>()),
throwsA(isA<PackageConfigParseError>()),
);
});

test(
'throws MissingPackageConfigError if package has a pubspec but no .dart_tool/package_config.json',
'throws PackageConfigParseError if package has a malformed .dart_tool/package_config.json',
() async {
final workspace = await createSimpleWorkspace(['package']);
workspace.dir('package', '.dart_tool').deleteSync(recursive: true);
workspace
.dir('package', '.dart_tool')
.file('package_config.json')
.writeAsStringSync('malformed');

expect(
() => fromContextRootsFromPaths(
Expand All @@ -2482,6 +2533,23 @@ dependency_overrides:
);
});

test('throws PubspecParseError if package has a malformed pubspec.yaml',
() async {
final workspace = await createSimpleWorkspace(['package']);
workspace
.dir('package')
.file('pubspec.yaml')
.writeAsStringSync('malformed');

expect(
() => fromContextRootsFromPaths(
[p.join(workspace.path, 'package')],
workingDirectory: workspace,
),
throwsA(isA<PubspecParseError>()),
);
});

test('Supports empty workspace', () async {
final customLintWorkspace = await fromContextRootsFromPaths(
[],
Expand Down
17 changes: 11 additions & 6 deletions packages/custom_lint_builder/lib/src/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -371,12 +371,17 @@ class _ClientAnalyzerPlugin extends analyzer_plugin.ServerPlugin {
// Otherwise tests may miss the first hot reload.
await _client._hotReloader;

final pubspecs = {
for (final analysisContext in contextCollection.contexts)
analysisContext: parsePubspec(
io.Directory(analysisContext.contextRoot.root.path),
),
};
final pubspecs = <AnalysisContext, Future<Pubspec>>{};

for (final context in contextCollection.contexts) {
final pubspec = tryFindProjectDirectory(
io.Directory(context.contextRoot.root.path),
);

if (pubspec != null) {
pubspecs[context] = parsePubspec(pubspec);
}
}

// Running before updating the configs as the config parsing depends
// on this operation.
Expand Down