-
Notifications
You must be signed in to change notification settings - Fork 75
Search for pubspecs surrounding analysis contexts #152
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
Changes from all commits
9f2ca24
c925ce8
deea5e6
33273a5
c375d98
3d080e2
d9f5609
5fd23c7
c46b021
c6071d7
736c968
6e171ff
78c4a8c
0455aa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
@@ -121,8 +122,82 @@ 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, | ||
bool missingPackageConfigOkay = false, | ||
}) { | ||
try { | ||
return findProjectDirectory( | ||
directory, | ||
original: original, | ||
missingPackageConfigOkay: missingPackageConfigOkay, | ||
); | ||
} on FileSystemException { | ||
return null; | ||
} | ||
} | ||
|
||
/// Finds the project directory associated with an analysis context root | ||
/// | ||
/// 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 findProjectDirectory( | ||
Directory directory, { | ||
Directory? original, | ||
bool missingPackageConfigOkay = false, | ||
}) { | ||
final packageConfigFile = directory.packageConfig.existsSync(); | ||
final pubspecFile = directory.pubspec.existsSync(); | ||
if (packageConfigFile && pubspecFile) { | ||
return directory; | ||
} | ||
if (pubspecFile) { | ||
if (missingPackageConfigOkay) { | ||
return directory; | ||
} | ||
throw PackageConfigNotFoundError._(directory.path); | ||
} | ||
if (directory.parent.uri == directory.uri) { | ||
throw FindProjectError._(original?.path ?? directory.path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather make the return value nullable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a tryFindProjectDirectory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not quite what I meant. I don't think we need a variant that throws. If we do want to throw, I'd move the exception to the place which calls |
||
} | ||
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; | ||
} | ||
|
||
/// No .dart_tool/package_config.json file was found for a plugin. | ||
@internal | ||
class PackageConfigNotFoundError extends FileSystemException { | ||
/// The [path] where the pubspec.yaml file was found, but missing a package_config.json | ||
PackageConfigNotFoundError._(String path) | ||
: super( | ||
'Failed to find .dart_tool/package_config.json at $path.\n' | ||
'Make sure to run `pub get` first.\n' | ||
'If "$path" is in your PUB_CACHE dir, run `dart pub cache repair`', | ||
path, | ||
); | ||
|
||
@override | ||
String toString() => message; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,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>{}; | ||
|
@@ -182,7 +183,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 | ||
|
@@ -208,8 +209,10 @@ Future<Directory> createSimpleWorkspace( | |
Future<Directory> createWorkspace( | ||
Map<String, Pubspec> pubspecs, { | ||
bool withPackageConfig = true, | ||
bool withNestedAnalysisOptions = false, | ||
bool local = false, | ||
}) async { | ||
final dir = createTemporaryDirectory(); | ||
final dir = createTemporaryDirectory(local: local); | ||
|
||
String packagePathOf(Dependency dependency, String name) { | ||
if (dependency is HostedDependency) { | ||
|
@@ -262,10 +265,16 @@ Future<Directory> createWorkspace( | |
return dir; | ||
} | ||
|
||
Directory createTemporaryDirectory() { | ||
final dir = Directory.current // | ||
.dir('.dart_tool') | ||
.createTempSync('custom_lint_test'); | ||
Directory createTemporaryDirectory({bool local = false}) { | ||
rrousselGit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
|
@@ -724,25 +733,55 @@ void main() { | |
); | ||
} | ||
|
||
test('throws MissingPubspecError if package does not contain a pubspec', | ||
TimWhiting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
test( | ||
TimWhiting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'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'), | ||
); | ||
analysisFile.createSync(); | ||
analysisFile.writeAsStringSync(analysisOptionsWithCustomLintEnabled); | ||
final nestedAnalysisFile = File( | ||
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)); | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you remove those trailing commas and format again? Tests typically don't use them |
||
); | ||
|
||
test( | ||
'throws PackageConfigNotFoundError if package has a pubspec but no .dart_tool/package_config.json', | ||
TimWhiting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
() 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<PackageConfigNotFoundError>()), | ||
); | ||
}); | ||
|
||
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( | ||
|
@@ -753,6 +792,23 @@ void main() { | |
); | ||
}); | ||
|
||
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( | ||
[], | ||
|
@@ -2019,8 +2075,8 @@ dart pub upgrade dep | |
'app2', | ||
devDependencies: { | ||
'dep': HostedDependency( | ||
version: VersionConstraint.parse('>=2.0.0 <3.0.0'), | ||
), | ||
version: VersionConstraint.parse('>=3.0.0 <4.0.0'), | ||
) | ||
}, | ||
), | ||
]); | ||
|
@@ -2062,7 +2118,7 @@ Plugin dep: | |
- Hosted with version constraint: ^1.0.0 | ||
Resolved with ${app.plugins.single.package.root.path} | ||
Used by project "app" at "app" | ||
- Hosted with version constraint: >=2.0.0 <3.0.0 | ||
- Hosted with version constraint: >=3.0.0 <4.0.0 | ||
Resolved with ${app2.plugins.single.package.root.path} | ||
Used by project "app2" at "app2" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handles looking for a project directory which contains at minimum a pubspec next to a .dart_tool/package_config.json (and in the case of hosted dependencies in the pubcache, the package config file is not necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we care about searching for the existence of a
package_config.json
here? Shouldn't searching only for apubspec
be enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this was resolved. I still don't get why we are looking for package_config.jsons
To begin with, there's a high chance that if there's a missing package_config, then #168 should take care of it.