-
Notifications
You must be signed in to change notification settings - Fork 75
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
Allow client to specify how to find the package config #2199
Conversation
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.
Thanks Elliott, left some comments.
dwds/lib/src/utilities/globals.dart
Outdated
void setPackageConfigPath(path) => _packageConfigPath = path; | ||
|
||
/// The default package config path, if none is provided by the load strategy. | ||
String get _defaultPackageConfigPath => p.join( |
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.
Can we add tests for a scenario where the package config is not located in a usual place?
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.
Done, added a load_strategy_test
for this
dwds/test/fixtures/fakes.dart
Outdated
@@ -368,7 +371,7 @@ class FakeStrategy implements LoadStrategy { | |||
MetadataProvider(entrypoint, FakeAssetReader()); | |||
|
|||
@override | |||
void trackEntrypoint(String entrypoint) {} | |||
Future<void> trackEntrypoint(String entrypoint) => Future.value(null); |
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.
Can this be Future<void> trackEntrypoint(String entrypoint) async {}
instead?
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.
Removed this because the FakeStrategy
now extends instead of implements the LoadStrategy
dwds/lib/src/utilities/globals.dart
Outdated
import 'package:path/path.dart' as p; | ||
|
||
/// The path to the package config. | ||
String get packageConfigPath => _packageConfigPath ?? _defaultPackageConfigPath; |
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.
Do we need a special global for this? Can it be just a part of the global strategy, to minimize the number of globals?
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.
Sure, moved to global strategy instead
Work towards #2198
The package config is not located at
.dart_tool/package_config.json
for google3 apps, therefore we need to provide a way for the code runner to specify where to look for it.CC @helin24