Skip to content

Commit

Permalink
Display an error message instead of crashing on wrong data type in da…
Browse files Browse the repository at this point in the history 
…rtdoc_options.yaml (#3372)
  • Loading branch information
@jcollins-g
jcollins-g authored Mar 23, 2023
1 parent a094d22 commit 15d1e25
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 61 deletions.
20 changes: 17 additions & 3 deletions lib/src/dartdoc_options.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,17 @@ abstract class DartdocOption<T extends Object?> {

bool get _isDouble => _kDoubleVal is T;

String get _expectedTypeForDisplay {
if (_isString) return 'String';
if (_isListString) return 'list of Strings';
if (_isMapString) return 'map of String to String';
if (_isBool) return 'boolean';
if (_isInt) return 'int';
if (_isDouble) return 'double';
assert(false, 'Expecting an unknown type');
return '<<unknown>>';
}

final Map<String, _YamlFileData> __yamlAtCanonicalPathCache = {};

/// Implementation detail for [DartdocOptionFileOnly]. Make sure we use
Expand Down Expand Up @@ -945,9 +956,8 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
};
}
if (convertYamlToType == null) {
throw DartdocOptionError(
'Unable to convert yaml to type for option: $fieldName, method not '
'defined');
throw UnsupportedError(
'$fieldName: convertYamlToType method not defined');
}
var canonicalDirectoryPath =
resourceProvider.pathContext.canonicalize(contextPath);
Expand All @@ -964,6 +974,10 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
} else {
throw UnsupportedError('Type $T is not supported');
}
if (returnData == null) {
throw DartdocOptionError('Error in dartdoc_options.yaml, $fieldName: '
'expecting a $_expectedTypeForDisplay, got `$yamlData`');
}
return _OptionValueWithContext(returnData as T, contextPath,
definingFile: 'dartdoc_options.yaml');
}
Expand Down
130 changes: 72 additions & 58 deletions test/dartdoc_options_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class ConvertedOption {

void main() {
var resourceProvider = pubPackageMetaProvider.resourceProvider;
var path = pubPackageMetaProvider.resourceProvider.pathContext;

late final DartdocOptionRoot dartdocOptionSetFiles;
late final DartdocOptionRoot dartdocOptionSetArgs;
Expand Down Expand Up @@ -174,34 +175,30 @@ void main() {
));

tempDir = resourceProvider.createSystemTemp('options_test');
firstDir = resourceProvider
.getFolder(resourceProvider.pathContext.join(tempDir.path, 'firstDir'))
firstDir = resourceProvider.getFolder(path.join(tempDir.path, 'firstDir'))
..create();
firstExisting = resourceProvider.getFile(
resourceProvider.pathContext.join(firstDir.path, 'existing.dart'))
firstExisting = resourceProvider
.getFile(path.join(firstDir.path, 'existing.dart'))
..writeAsStringSync('');
secondDir = resourceProvider
.getFolder(resourceProvider.pathContext.join(tempDir.path, 'secondDir'))
secondDir = resourceProvider.getFolder(path.join(tempDir.path, 'secondDir'))
..create();
resourceProvider
.getFile(
resourceProvider.pathContext.join(secondDir.path, 'existing.dart'))
.getFile(path.join(secondDir.path, 'existing.dart'))
.writeAsStringSync('');

secondDirFirstSub = resourceProvider.getFolder(
resourceProvider.pathContext.join(secondDir.path, 'firstSub'))
secondDirFirstSub = resourceProvider
.getFolder(path.join(secondDir.path, 'firstSub'))
..create();
secondDirSecondSub = resourceProvider.getFolder(
resourceProvider.pathContext.join(secondDir.path, 'secondSub'))
secondDirSecondSub = resourceProvider
.getFolder(path.join(secondDir.path, 'secondSub'))
..create();

dartdocOptionsOne = resourceProvider.getFile(resourceProvider.pathContext
.join(firstDir.path, 'dartdoc_options.yaml'));
dartdocOptionsTwo = resourceProvider.getFile(resourceProvider.pathContext
.join(secondDir.path, 'dartdoc_options.yaml'));
var dartdocOptionsTwoFirstSub = resourceProvider.getFile(resourceProvider
.pathContext
.join(secondDirFirstSub.path, 'dartdoc_options.yaml'));
dartdocOptionsOne = resourceProvider
.getFile(path.join(firstDir.path, 'dartdoc_options.yaml'));
dartdocOptionsTwo = resourceProvider
.getFile(path.join(secondDir.path, 'dartdoc_options.yaml'));
var dartdocOptionsTwoFirstSub = resourceProvider
.getFile(path.join(secondDirFirstSub.path, 'dartdoc_options.yaml'));

dartdocOptionsOne.writeAsStringSync('''
dartdoc:
Expand Down Expand Up @@ -247,6 +244,42 @@ dartdoc:
}
});

group('invalid data in yaml', () {
late Folder invalid;
late File dartdocOptionsInvalid;

setUp(() {
invalid = resourceProvider.getFolder(path.join(tempDir.path, 'invalid'))
..create();
dartdocOptionsInvalid = resourceProvider
.getFile(path.join(invalid.path, 'dartdoc_options.yaml'));
dartdocOptionsInvalid.writeAsStringSync('''
dartdoc:
categoryOrder: 'options_one'
''');
});

tearDown(() {
try {
invalid.delete();
} catch (e) {
print('Ignoring error trying to delete temp: $e');
}
});

test('validate correct throw for wrong data type', () {
dartdocOptionSetFiles.parseArguments([]);
expect(() {
dartdocOptionSetFiles['categoryOrder'].valueAt(invalid);
},
throwsA(const TypeMatcher<DartdocOptionError>().having(
(e) => e.message,
'message',
equals('Error in dartdoc_options.yaml, dartdoc.categoryOrder: '
'expecting a list of Strings, got `options_one`'))));
});
});

group('new style synthetic option', () {
test('validate argument override changes value', () {
dartdocOptionSetSynthetic.parseArguments(['--my-special-integer', '12']);
Expand All @@ -272,10 +305,8 @@ dartdoc:
} on DartdocFileMissing catch (e) {
errorMessage = e.message;
}
var missingPath = resourceProvider.pathContext.canonicalize(
resourceProvider.pathContext.join(
resourceProvider.pathContext.absolute(tempDir.path),
'existing.dart'));
var missingPath = path.canonicalize(
path.join(path.absolute(tempDir.path), 'existing.dart'));
expect(
errorMessage,
equals('Synthetic configuration option vegetableLoaderChecked from '
Expand All @@ -296,7 +327,7 @@ dartdoc:
// Since this is an ArgSynth, it ignores the yaml option and resolves to the CWD
expect(
dartdocOptionSetSynthetic['nonCriticalFileOption'].valueAt(firstDir),
equals(resourceProvider.pathContext.canonicalize('stuff.zip')));
equals(path.canonicalize('stuff.zip')));
});

test('ArgSynth defaults to synthetic', () {
Expand All @@ -320,10 +351,8 @@ dartdoc:
} on DartdocFileMissing catch (e) {
errorMessage = e.message;
}
var missingPath = resourceProvider.pathContext.join(
resourceProvider.pathContext
.canonicalize(resourceProvider.pathContext.current),
'override-not-existing.dart');
var missingPath = path.join(
path.canonicalize(path.current), 'override-not-existing.dart');
expect(
errorMessage,
equals('Argument --file-option, set to override-not-existing.dart, '
Expand Down Expand Up @@ -386,20 +415,15 @@ dartdoc:
});

group('glob options', () {
String canonicalize(String path) =>
resourceProvider.pathContext.canonicalize(path);
String canonicalize(String inputPath) => path.canonicalize(inputPath);

test('work via the command line', () {
dartdocOptionSetAll
.parseArguments(['--glob-option', p.join('foo', '**')]);
expect(
dartdocOptionSetAll['globOption'].valueAtCurrent(),
equals([
resourceProvider.pathContext.joinAll([
canonicalize(resourceProvider.pathContext.current),
'foo',
'**'
])
path.joinAll([canonicalize(path.current), 'foo', '**'])
]));
});

Expand All @@ -408,25 +432,21 @@ dartdoc:
expect(
dartdocOptionSetAll['globOption'].valueAt(secondDir),
equals([
canonicalize(
resourceProvider.pathContext.join(secondDir.path, 'q*.html')),
canonicalize(
resourceProvider.pathContext.join(secondDir.path, 'e*.dart')),
canonicalize(path.join(secondDir.path, 'q*.html')),
canonicalize(path.join(secondDir.path, 'e*.dart')),
]));
// No child override, should be the same as parent
expect(
dartdocOptionSetAll['globOption'].valueAt(secondDirSecondSub),
equals([
canonicalize(
resourceProvider.pathContext.join(secondDir.path, 'q*.html')),
canonicalize(
resourceProvider.pathContext.join(secondDir.path, 'e*.dart')),
canonicalize(path.join(secondDir.path, 'q*.html')),
canonicalize(path.join(secondDir.path, 'e*.dart')),
]));
// Child directory overrides
expect(
dartdocOptionSetAll['globOption'].valueAt(secondDirFirstSub),
equals([
resourceProvider.pathContext.joinAll(
path.joinAll(
[canonicalize(secondDirFirstSub.path), '**', '*.dart'])
]));
});
Expand Down Expand Up @@ -464,10 +484,8 @@ dartdoc:
} on DartdocFileMissing catch (e) {
errorMessage = e.message;
}
var missingPath = resourceProvider.pathContext.join(
resourceProvider.pathContext
.canonicalize(resourceProvider.pathContext.current),
'not_found.txt');
var missingPath =
path.join(path.canonicalize(path.current), 'not_found.txt');
expect(
errorMessage,
equals('Argument --single-file, set to not_found.txt, resolves to '
Expand All @@ -478,7 +496,7 @@ dartdoc:
String? errorMessage;
dartdocOptionSetArgs.parseArguments([
'--files-flag',
resourceProvider.pathContext.absolute(firstExisting.path),
path.absolute(firstExisting.path),
'--files-flag',
'other_not_found.txt'
]);
Expand All @@ -487,14 +505,12 @@ dartdoc:
} on DartdocFileMissing catch (e) {
errorMessage = e.message;
}
var missingPath = resourceProvider.pathContext.join(
resourceProvider.pathContext
.canonicalize(resourceProvider.pathContext.current),
'other_not_found.txt');
var missingPath =
path.join(path.canonicalize(path.current), 'other_not_found.txt');
expect(
errorMessage,
equals('Argument --files-flag, set to '
'[${resourceProvider.pathContext.absolute(firstExisting.path)}, '
'[${path.absolute(firstExisting.path)}, '
'other_not_found.txt], resolves to missing path: '
'"$missingPath"'));
});
Expand All @@ -506,10 +522,8 @@ dartdoc:
.parseArguments(['--unimportant-file', 'this-will-never-exist']);
expect(
dartdocOptionSetArgs['unimportantFile'].valueAt(tempDir),
equals(resourceProvider.pathContext.join(
resourceProvider.pathContext
.canonicalize(resourceProvider.pathContext.current),
'this-will-never-exist')));
equals(path.join(
path.canonicalize(path.current), 'this-will-never-exist')));
});

test('DartdocOptionArgOnly has correct flag names', () {
Expand Down

0 comments on commit 15d1e25

Please sign in to comment.