[flutter_plugin_tools] Introduce a class for packages #4252
Conversation
|
|
||
| await initializeRun(); | ||
|
|
||
| final List<PackageEnumerationEntry> packages = includeSubpackages | ||
| final List<PackageEnumerationEntry> packageEnumeration = includeSubpackages |
There was a problem hiding this comment.
nit: s/packageEnumeration/targetPackages I wouldn't call something an enumeration unless its purpose is to assign a number to items, plus targetPackages matches the verbiage of the function.
There was a problem hiding this comment.
Done. (I actually made this change initially because you'd expressed concern about referring to a PackageEnumerationEntry as a package in the earlier review discussion; I like targetPackages better myself.)
As for enumeration, I was using it in what I think of as the common CS sense (Wikipedia: "An enumeration is a complete, ordered listing of all the items in a collection. The term is commonly used in mathematics and computer science to refer to a listing of all of the elements of a set.")
|
|
||
| /// The package's location. | ||
| final Directory directory; | ||
| /// The package. |
There was a problem hiding this comment.
nit: not really a helpful comment.
There was a problem hiding this comment.
I added a warning about excluded to this, as making sure that's not missed seemed like the only useful thing to say here. (I have to say something, because the repo has the lint requiring comments on public fields.
| {required String package}) async { | ||
| assert(package.isNotEmpty); | ||
| {required String packageName}) async { | ||
| assert(packageName.isNotEmpty); |
There was a problem hiding this comment.
fyi: It would be nice if Dart could statically assert this. In many places in my code I just traded null checks for isEmpty checks.
There was a problem hiding this comment.
I just traded null checks for isEmpty checks
Well, not really; you traded code that was wrong without you noticing for code that is correct 🙂. Because if an empty string wasn't valid, but the check you had before was foo != null rather than foo != null && !foo.isEmpty, then the old code was wrong.
| RepositoryPackage(this.directory); | ||
|
|
||
| /// The location of the package. | ||
| Directory directory; |
There was a problem hiding this comment.
I think RepositoryPackage should be immutable.
I was going to do that when I first wrote it, but one of the things I want to add to this later (per the TODO) is a lazy cached accessor for the parsed pubspec, as that's something that's used in several places (and caching it would improve some APIs that are currently passing around the package and its parsed pubspec to avoid re-parsing). That doesn't work if it's immutable, I don't want to thrash on adding and then later removing all the consts at creation sites when removing immutability at that point.
|
|
||
| await initializeRun(); | ||
|
|
||
| final List<PackageEnumerationEntry> packages = includeSubpackages | ||
| final List<PackageEnumerationEntry> packageEnumeration = includeSubpackages |
There was a problem hiding this comment.
Done. (I actually made this change initially because you'd expressed concern about referring to a PackageEnumerationEntry as a package in the earlier review discussion; I like targetPackages better myself.)
As for enumeration, I was using it in what I think of as the common CS sense (Wikipedia: "An enumeration is a complete, ordered listing of all the items in a collection. The term is commonly used in mathematics and computer science to refer to a listing of all of the elements of a set.")
|
|
||
| /// The package's location. | ||
| final Directory directory; | ||
| /// The package. |
There was a problem hiding this comment.
I added a warning about excluded to this, as making sure that's not missed seemed like the only useful thing to say here. (I have to say something, because the repo has the lint requiring comments on public fields.
| RepositoryPackage(this.directory); | ||
|
|
||
| /// The location of the package. | ||
| Directory directory; |
Yea makes sense. When I saw those methods I thought memoizing them could be helpful, but ultimately I'm not sure how many times you call them. Immutable minus whatever is needed for memoization sounds good to me. |
Packages are the primary conceptual object in the tool, but currently they are represented simply as Directory (or occasionally a path string). This introduces an object for packages and: - moves a number of existing utility methods into it - sweeps the code for the obvious cases of using `Directory` to represent a package, especially in method signatures and migrates them - notes a few places where we should migrate later, to avoid ballooning the size of the PR There are no doubt other cases not caught in the sweep, but this gives us a foundation both for new code, and to migrate incrementally toward as we find existing code that was missed.
Packages are the primary conceptual object in the tool, but currently they are represented simply as Directory (or occasionally a path string). This introduces an object for packages and: - moves a number of existing utility methods into it - sweeps the code for the obvious cases of using `Directory` to represent a package, especially in method signatures and migrates them - notes a few places where we should migrate later, to avoid ballooning the size of the PR There are no doubt other cases not caught in the sweep, but this gives us a foundation both for new code, and to migrate incrementally toward as we find existing code that was missed.
Packages are the primary conceptual object in the tool, but currently they are represented simply as Directory (or occasionally a path string). This introduces an object for packages and:
Directoryto represent a package, especially in method signatures and migrates themThere are no doubt other cases not caught in the sweep, but this gives us a foundation both for new code, and to migrate incrementally toward as we find existing code that was missed.
Pre-launch Checklist
dart format.)[shared_preferences]///).