Simplify package_meta.dart #3572
Conversation
| /// | ||
| /// For a given directory to be detected as an SDK, at least one of the given | ||
| /// paths must exist, for each list of paths. | ||
| // Update `_writeMockSdkBinFiles` in `test/src/utils.dart` if removing any |
There was a problem hiding this comment.
No, this is just supposed to be a code comment. Not part of the documentation (a little silly since its a private element).
| @@ -104,6 +91,8 @@ class PackageMetaProvider { | |||
| /// | |||
| /// Overriding this is typically done by overriding factories as rest of | |||
| /// `dartdoc` creates this object by calling these static factories. | |||
| // This class has a single direct subclass in this package, [PubPackageMeta], | |||
There was a problem hiding this comment.
I intended for that to be just an aside, not part of the doc comment. It's a little silly since we don't have clients really, but the docs are on pub. The sentence just didn't seem appropriate for a doc comment.
| reasons.add('no name found in pubspec.yaml'); | ||
| } | ||
| return reasons; | ||
| return [ |
There was a problem hiding this comment.
It feels a little weird how this List and its conditional values are regenerated every time we want to check isValid. If _pubspec is final, could we not have just saved this as a field or something?
There was a problem hiding this comment.
It should be easy to compute this value, even if multiple times (I think it's only a few times per package; not like a few times per field-being-documented or something.
We're following "Avoid storing what you can calculate" here. (Not that it would take much storage, one field per package as well.)
What I'd like to do is rename it and make it a getter (so it could be a field (without creating a private backing field)), but that is a breaking change with google3 so... too much work.
There was a problem hiding this comment.
Fair enough. Thanks for the explanation!
A few things:
PackageMetaProvider.getMessageForMissingPackageMetawas a very round-about way to provide some text to an error-message. It turns out every single call site referenced the same message, so just tear down all of the redirection.PackageMeta.pathContextprivate.PackageMetaProvider.fromDirwithputIfAbsent.Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.