Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify package_meta.dart #3572

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Simplify package_meta.dart #3572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d4cf1e
Simplify package_meta.dart
srawlins Nov 5, 2023
806bde7
regenerate
srawlins Nov 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/src/generator/templates.runtime_renderers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17030,7 +17030,6 @@ const _invisibleGetters = {
'isSdk',
'isValid',
'name',
'pathContext',
'requiresFlutter',
'resolvedDir',
'resourceProvider',
Expand Down
10 changes: 8 additions & 2 deletions lib/src/model/package_graph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,14 @@ class PackageGraph with CommentReferable, Nameable, ModelBuilder {
var packageMeta =
packageMetaProvider.fromElement(libraryElement, config.sdkDir);
if (packageMeta == null) {
throw DartdocFailure(packageMetaProvider.getMessageForMissingPackageMeta(
libraryElement, config));
var libraryPath = libraryElement.librarySource.fullName;
var dartOrFlutter = config.flutterRoot == null ? 'dart' : 'flutter';
throw DartdocFailure(
"Unknown package for library: '$libraryPath'. Consider "
'`$dartOrFlutter pub get` and/or '
'`$dartOrFlutter pub global deactivate dartdoc` followed by '
'`$dartOrFlutter pub global activate dartdoc` to fix. Also, be sure '
'that `$dartOrFlutter analyze` completes without errors.');
}
var package = Package.fromPackageMeta(packageMeta, this);
var lib = Library.fromLibraryResult(resolvedLibrary, this, package);
Expand Down
132 changes: 49 additions & 83 deletions lib/src/package_meta.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
// ignore: implementation_imports
import 'package:analyzer/src/generated/sdk.dart' show DartSdk;
import 'package:dartdoc/src/dartdoc_options.dart';
import 'package:dartdoc/src/failure.dart';
import 'package:meta/meta.dart';
import 'package:path/path.dart' as path;
Expand All @@ -23,9 +22,13 @@ class PackageMetaFailure extends DartdocFailure {
PackageMetaFailure(super.message);
}

/// For each list in this list, at least one of the given paths must exist
/// for this to be detected as an SDK. Update `_writeMockSdkBinFiles` in
/// `test/src/utils.dart` if removing any entries here.
/// Various relative paths that indicate that a root direoctory is an SDK (Dart
/// or Flutter).
///
/// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// for line 30 and 31?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is just supposed to be a code comment. Not part of the documentation (a little silly since its a private element).

// entries here.
const List<List<String>> _sdkDirFilePathsPosix = [
['bin/dart.bat', 'bin/dart.exe', 'bin/dart'],
['include/dart_version.h'],
Expand All @@ -43,7 +46,6 @@ final PackageMetaProvider pubPackageMetaProvider = PackageMetaProvider(
.parent
.parent,
Platform.environment,
messageForMissingPackageMeta: PubPackageMeta.messageForMissingPackageMeta,
);

/// Sets the supported way of constructing [PackageMeta] objects.
Expand All @@ -65,9 +67,6 @@ class PackageMetaProvider {
final DartSdk? defaultSdk;
final Map<String, String> environmentProvider;

final String Function(LibraryElement, DartdocOptionContext)
_messageForMissingPackageMeta;

PackageMetaProvider(
this._fromElement,
this._fromFilename,
Expand All @@ -76,24 +75,12 @@ class PackageMetaProvider {
this.defaultSdkDir,
this.environmentProvider, {
this.defaultSdk,
String Function(LibraryElement, DartdocOptionContext)?
messageForMissingPackageMeta,
}) : _messageForMissingPackageMeta = messageForMissingPackageMeta ??
_defaultMessageForMissingPackageMeta;
});

PackageMeta? fromElement(LibraryElement library, String s) =>
_fromElement(library, s, resourceProvider);
PackageMeta? fromFilename(String s) => _fromFilename(s, resourceProvider);
PackageMeta? fromDir(Folder dir) => _fromDir(dir, resourceProvider);

String getMessageForMissingPackageMeta(
LibraryElement library, DartdocOptionContext optionContext) =>
_messageForMissingPackageMeta(library, optionContext);

static String _defaultMessageForMissingPackageMeta(
LibraryElement library, DartdocOptionContext optionContext) {
return 'Unknown package for library: ${library.source.fullName}';
}
}

/// Describes a single package in the context of `dartdoc`.
Expand All @@ -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],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for here ///?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// but has other subclasses in google3.
abstract class PackageMeta {
final Folder dir;

Expand All @@ -112,25 +101,22 @@ abstract class PackageMeta {
PackageMeta(this.dir, this.resourceProvider);

@override
bool operator ==(Object other) {
if (other is! PackageMeta) return false;
var otherMeta = other;
return resourceProvider.pathContext.equals(dir.path, otherMeta.dir.path);
}
bool operator ==(Object other) =>
other is PackageMeta && _pathContext.equals(dir.path, other.dir.path);

@override
int get hashCode => pathContext.hash(pathContext.absolute(dir.path));
int get hashCode => _pathContext.hash(_pathContext.absolute(dir.path));

path.Context get pathContext => resourceProvider.pathContext;
path.Context get _pathContext => resourceProvider.pathContext;

/// Returns true if this represents a 'Dart' SDK.
/// Whether this represents a 'Dart' SDK.
///
/// A package can be part of Dart and Flutter at the same time, but if we are
/// A package can be part of Dart and Flutter at the same time, but if this is
/// part of a Dart SDK, [sdkType] should never return null.
bool get isSdk;

/// Returns 'Dart' or 'Flutter' (preferentially, 'Flutter' when the answer is
/// "both"), or null if this package is not part of a SDK.
/// "both"), or `null` if this package is not part of an SDK.
String? sdkType(String? flutterRootPath);

bool get requiresFlutter;
Expand All @@ -147,14 +133,12 @@ abstract class PackageMeta {

File? getReadmeContents();

/// Returns true if we are a valid package, valid enough to generate docs.
/// Whether this is a valid package (valid enough to generate docs).
bool get isValid => getInvalidReasons().isEmpty;

/// Returns resolved directory.
String get resolvedDir;

/// Returns a list of reasons this package is invalid, or an
/// empty list if no reasons found.
/// The list of reasons this package is invalid.
///
/// If the list is empty, this package is valid.
List<String> getInvalidReasons();
Expand All @@ -179,8 +163,8 @@ abstract class PubPackageMeta extends PackageMeta {

static final _sdkDirParent = <String, Folder?>{};

/// If [folder] is inside a Dart SDK, returns the directory of the SDK, and `null`
/// otherwise.
/// If [folder] is inside a Dart SDK, returns the directory of the SDK, and
/// `null` otherwise.
static Folder? sdkDirParent(
Folder folder, ResourceProvider resourceProvider) {
var pathContext = resourceProvider.pathContext;
Expand All @@ -200,7 +184,7 @@ abstract class PubPackageMeta extends PackageMeta {
return _sdkDirParent[dirPathCanonical];
}

/// Use this instead of fromDir where possible.
/// Use this instead of [fromDir] where possible.
static PackageMeta? fromElement(LibraryElement libraryElement, String sdkDir,
ResourceProvider resourceProvider) {
if (libraryElement.isInSdk) {
Expand All @@ -223,64 +207,49 @@ abstract class PubPackageMeta extends PackageMeta {

/// This factory is guaranteed to return the same object for any given
/// `dir.absolute.path`. Multiple `dir.absolute.path`s will resolve to the
/// same object if they are part of the same package. Returns null
/// if the directory is not part of a known package.
/// same object if they are part of the same package. Returns `null` if the
/// directory is not part of a known package.
static PackageMeta? fromDir(
Folder folder, ResourceProvider resourceProvider) {
var pathContext = resourceProvider.pathContext;
var original =
resourceProvider.getFolder(pathContext.absolute(folder.path));
folder = original;
if (!original.exists) {
folder = resourceProvider.getFolder(pathContext.absolute(folder.path));
if (!folder.exists) {
throw PackageMetaFailure(
'fatal error: unable to locate the input directory at '
"'${original.path}'.");
'fatal error: unable to locate the input directory at '
"'${folder.path}'.",
);
}

if (!_packageMetaCache.containsKey(folder.path)) {
PackageMeta? packageMeta;
return _packageMetaCache.putIfAbsent(pathContext.absolute(folder.path), () {
// There are pubspec.yaml files inside the SDK. Ignore them.
var parentSdkDir = sdkDirParent(folder, resourceProvider);
if (parentSdkDir != null) {
packageMeta = _SdkMeta(parentSdkDir, resourceProvider);
return _SdkMeta(parentSdkDir, resourceProvider);
} else {
for (var dir in folder.withAncestors) {
var pubspec = resourceProvider
.getFile(pathContext.join(dir.path, 'pubspec.yaml'));
if (pubspec.exists) {
packageMeta = _FilePackageMeta(dir, resourceProvider);
break;
return _FilePackageMeta(dir, resourceProvider);
}
}
}
_packageMetaCache[pathContext.absolute(folder.path)] = packageMeta;
}
return _packageMetaCache[pathContext.absolute(folder.path)];
}

/// Create a helpful user error message for a case where a package can not
/// be found.
static String messageForMissingPackageMeta(
LibraryElement library, DartdocOptionContext optionContext) {
var libraryString = library.librarySource.fullName;
var dartOrFlutter = optionContext.flutterRoot == null ? 'dart' : 'flutter';
return 'Unknown package for library: $libraryString. Consider `$dartOrFlutter pub get` and/or '
'`$dartOrFlutter pub global deactivate dartdoc` followed by `$dartOrFlutter pub global activate dartdoc` to fix. '
'Also, be sure that `$dartOrFlutter analyze` completes without errors.';
return null;
});
}

@override
String? sdkType(String? flutterRootPath) {
if (flutterRootPath != null) {
var flutterPackages = pathContext.join(flutterRootPath, 'packages');
var flutterBinCache = pathContext.join(flutterRootPath, 'bin', 'cache');
var flutterPackages = _pathContext.join(flutterRootPath, 'packages');
var flutterBinCache = _pathContext.join(flutterRootPath, 'bin', 'cache');

/// Don't include examples or other non-SDK components as being the
/// "Flutter SDK".
var canonicalizedDir = pathContext
var canonicalizedDir = _pathContext
.canonicalize(resourceProvider.pathContext.absolute(dir.path));
if (pathContext.isWithin(flutterPackages, canonicalizedDir) ||
pathContext.isWithin(flutterBinCache, canonicalizedDir)) {
if (_pathContext.isWithin(flutterPackages, canonicalizedDir) ||
_pathContext.isWithin(flutterBinCache, canonicalizedDir)) {
return 'Flutter';
}
}
Expand Down Expand Up @@ -313,17 +282,17 @@ class _FilePackageMeta extends PubPackageMeta {
// possibly by calculating hosting directly from pubspec.yaml or importing
// a pub library to do this.
// People could have a pub cache at root with Windows drive mappings.
if (pathContext.split(pathContext.canonicalize(dir.path)).length >= 3) {
if (_pathContext.split(_pathContext.canonicalize(dir.path)).length >= 3) {
var pubCacheRoot = dir.parent.parent.parent.path;
// Check for directory structure too close to root.
if (pubCacheRoot != dir.parent.parent.path) {
var hosted = pathContext.canonicalize(dir.parent.parent.path);
var hostname = pathContext.canonicalize(dir.parent.path);
if (pathContext.basename(hosted) == 'hosted' &&
var hosted = _pathContext.canonicalize(dir.parent.parent.path);
var hostname = _pathContext.canonicalize(dir.parent.path);
if (_pathContext.basename(hosted) == 'hosted' &&
resourceProvider
.getFolder(pathContext.join(pubCacheRoot, '_temp'))
.getFolder(_pathContext.join(pubCacheRoot, '_temp'))
.exists) {
hostedAt = pathContext.basename(hostname);
hostedAt = _pathContext.basename(hostname);
}
}
}
Expand Down Expand Up @@ -360,13 +329,10 @@ class _FilePackageMeta extends PubPackageMeta {
/// empty list if no reasons found.
@override
List<String> getInvalidReasons() {
var reasons = <String>[];
if (_pubspec.isEmpty) {
reasons.add('no pubspec.yaml found');
} else if (!_pubspec.containsKey('name')) {
reasons.add('no name found in pubspec.yaml');
}
return reasons;
return [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Thanks for the explanation!

if (_pubspec.isEmpty) 'no pubspec.yaml found',
if (!_pubspec.containsKey('name')) "no 'name' field found in pubspec.yaml"
];
}
}

Expand Down
10 changes: 0 additions & 10 deletions test/end2end/model_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,6 @@ void main() {
packageGraph.libraries.firstWhere((lib) => lib.name == 'base_class');
});

group('PackageMeta and PackageGraph integration', () {
test('PackageMeta error messages generate correctly', () {
var message = packageGraph.packageMetaProvider
.getMessageForMissingPackageMeta(
fakeLibrary.element, packageGraph.config);
expect(message, contains('fake.dart'));
expect(message, contains('pub global activate dartdoc'));
});
});

group('triple-shift', () {
Library tripleShift;
late final Class C, E, F;
Expand Down
1 change: 0 additions & 1 deletion test/src/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ PackageMetaProvider get testPackageMetaProvider {
sdkRoot,
{},
defaultSdk: FolderBasedDartSdk(resourceProvider, sdkRoot),
messageForMissingPackageMeta: PubPackageMeta.messageForMissingPackageMeta,
);
}

Expand Down