Placeholder package config reader. #29
Conversation
davidmorgan
force-pushed
the
read-pubspec
branch
from
8f99484
to
781a3e4
Compare
| packageName = packageName.substring('package:'.length); | ||
| packageName = packageName.substring(0, packageName.indexOf('/')); | ||
| } else { | ||
| throw ArgumentError('Name must start "package:" and have a path: $name'); |
There was a problem hiding this comment.
We should have a TODO here to look up the package name by URI in the package config, to support macros defined outside of lib dirs.
| import 'package:package_config/package_config.dart' as package_config; | ||
|
|
||
| /// Reads a package config to determine information about macros. | ||
| class PackageConfig { |
There was a problem hiding this comment.
Lets also call this something other than PackageConfig. Really, it is something that reads a package config and extracts information about macros from it.
There was a problem hiding this comment.
I guess: MacroPackageConfig :) done.
|
|
||
| /// Reads a package config to determine information about macros. | ||
| class PackageConfig { | ||
| final Uri uri; |
There was a problem hiding this comment.
This should probably just take in a PackageConfig instance - and have a factory constructor or static method which can construct that from a URI. That will make it more testable and avoid re-reading/parsing it on every call to lookupMacroImplementation.
| final pubspecUri = packageUri.replace( | ||
| pathSegments: | ||
| packageUri.pathSegments.followedBy(['pubspec.yaml']).toList()); |
There was a problem hiding this comment.
You should be able to just do:
| final pubspecUri = packageUri.replace( | |
| pathSegments: | |
| packageUri.pathSegments.followedBy(['pubspec.yaml']).toList()); | |
| final pubspecUri = packageUri.resolve('pubspec.yaml'); |
There was a problem hiding this comment.
I definitely tried that! But now I try it again it works. Weird. I wonder how I screwed up :)
Thanks.
There was a problem hiding this comment.
Possibly you didn't have a trailing / originally for the root URI. Afaik though the package config spec requires this trailing slash.
| } | ||
| final packageConfig = package_config.PackageConfig.parseBytes( | ||
| File.fromUri(uri).readAsBytesSync(), uri); | ||
| final libraryQualifiedName = |
There was a problem hiding this comment.
nit: This is really a path not just a name. It also doesn't include the lib/ which it should, to allow macros to be defined outside of lib/ (and below, I would expect the full path not the lib relative path in the # macro comments.
davidmorgan
force-pushed
the
read-pubspec
branch
4 times, most recently
from
ffa1ad0
to
a5f0f58
Compare
|
Thanks! Comments addressed, and I added another commit that actually uses the new code, PTAL. |
| final matchingPackage = packageConfig.packages | ||
| .where((p) => p.name == packageName) | ||
| .toList() | ||
| .singleOrNull; |
There was a problem hiding this comment.
I am sure it doesn't matter all that much, but creating a closure + iterable + list here does bug me :).
Luckily, PackageConfig implements the [] operator :).
| final matchingPackage = packageConfig.packages | |
| .where((p) => p.name == packageName) | |
| .toList() | |
| .singleOrNull; | |
| final matchingPackage = packageConfig[packageName]; |
davidmorgan
force-pushed
the
read-pubspec
branch
from
a5f0f58
to
52b7e98
Compare
davidmorgan
force-pushed
the
read-pubspec
branch
from
52b7e98
to
91b9e58
Compare
davidmorgan
force-pushed
the
read-pubspec
branch
from
91b9e58
to
ad094d5
Compare
|
Rebasing on top of the CFE PR it turns out the package config class has exactly what I need to remove the hack over there, resolving |
So we can identify macros based on pubspecs while we wait for that to be implemented properly.
Not used yet as that would overlap with other pending PRs, but it should be easy to plug in afterwards.