Skip to content

Better error unknown #16

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.

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

Merged
merged 6 commits into from
Jul 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 0 additions & 7 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,10 @@ sudo: false
dart:
- dev
- stable
- 1.22.1
dart_task:
- test
- dartfmt
- dartanalyzer
matrix:
exclude:
- dart: 1.22.1
dart_task: dartfmt
- dart: 1.22.1
dart_task: dartanalyzer

# Only building master means that we don't run two builds for each pull request.
branches:
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.1.1

* Fail generation when undefined types are encountered.
Throw a helpful error.

## 0.1.0+1

* Fix homepage in `pubspec.yaml`.
Expand Down
34 changes: 28 additions & 6 deletions lib/src/json_serializable_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ class JsonSerializableGenerator
// TODO: support overriding the field set with an annotation option
var fieldsList = classElement.fields.where((e) => !e.isStatic).toList();

var undefinedFields =
fieldsList.where((fe) => fe.type.isUndefined).toList();
Copy link
Member

Choose a reason for hiding this comment

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

why do you need toList?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hate reiterating iterables – even trivially. Can have unintended perf and behavior affects.

So I just never do it.

if (undefinedFields.isNotEmpty) {
var description =
undefinedFields.map((fe) => "`${fe.displayName}`").join(', ');

throw new InvalidGenerationSourceError(
'At least one field has an invalid type: $description.',
todo: 'Check names and imports.');
}

// Sort these in the order in which they appear in the class
// Sadly, `classElement.fields` puts properties after fields
fieldsList.sort((a, b) => a.nameOffset.compareTo(b.nameOffset));
Expand Down Expand Up @@ -151,6 +162,19 @@ class JsonSerializableGenerator
fieldsToSet.remove(arg.name);
}

var undefinedArgs = [ctorArguments, ctorNamedArguments]
.expand((l) => l)
Copy link
Member

Choose a reason for hiding this comment

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

we need Iterable.followedBy :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed!

.where((pe) => pe.type.isUndefined)
.toList();
Copy link
Member

Choose a reason for hiding this comment

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

why toList()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

if (undefinedArgs.isNotEmpty) {
var description =
undefinedArgs.map((fe) => "`${fe.displayName}`").join(', ');

throw new InvalidGenerationSourceError(
'At least one constructor argument has an invalid type: $description.',
todo: 'Check names and imports.');
}

// these are fields to skip – now to find them
var finalFields =
fieldsToSet.values.where((field) => field.isFinal).toSet();
Expand Down Expand Up @@ -296,15 +320,13 @@ class JsonSerializableGenerator
{ParameterElement ctorParam}) {
name = _fieldToJsonMapKey(name, field);
var result = "json['$name']";
return _writeAccessToJsonValue(result, field.type, ctorParam: ctorParam);

var targetType = ctorParam?.type ?? field.type;
return _writeAccessToJsonValue(result, targetType);
}

String _writeAccessToJsonValue(String varExpression, DartType searchType,
{ParameterElement ctorParam, int depth: 0}) {
if (ctorParam != null) {
searchType = ctorParam.type as InterfaceType;
}

{int depth: 0}) {
for (var helper in _typeHelpers) {
if (helper.canDeserialize(searchType)) {
return "$varExpression == null ? null : "
Expand Down
4 changes: 2 additions & 2 deletions pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
name: json_serializable
version: 0.1.0+1
version: 0.1.1-dev
author: Dart Team <misc@dartlang.org>
description: Generates utilities to aid in serializing to/from JSON.
homepage: https://github.com/dart-lang/json_serializable
environment:
sdk: '>=1.22.1 <2.0.0-dev.infinity'
sdk: '>=1.24.0 <2.0.0-dev.infinity'
dependencies:
analyzer: '>=0.29.10 <0.31.0'
build: ^0.9.0
Expand Down
56 changes: 46 additions & 10 deletions test/json_serializable_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import 'dart:async';

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/generated/source_io.dart';
import 'package:analyzer/src/string_source.dart';
import 'package:json_serializable/json_serializable.dart';
import 'package:path/path.dart' as p;
Expand All @@ -25,17 +23,43 @@ void main() {
test('const field', () async {
var element = await _getClassForCodeString('theAnswer');

expect(_generator.generate(element, null),
throwsInvalidGenerationSourceError);
// TODO: validate the properties on the thrown error
expect(
_generator.generate(element, null),
throwsInvalidGenerationSourceError(
'Generator cannot target `const dynamic theAnswer`.',
'Remove the JsonSerializable annotation from `const dynamic theAnswer`.'));
});

test('method', () async {
var element = await _getClassForCodeString('annotatedMethod');

expect(_generator.generate(element, null),
throwsInvalidGenerationSourceError);
// TODO: validate the properties on the thrown error
expect(
_generator.generate(element, null),
throwsInvalidGenerationSourceError(
'Generator cannot target `annotatedMethod`.',
'Remove the JsonSerializable annotation from `annotatedMethod`.'));
});
});

group('unknown types', () {
test('in constructor arguments', () async {
var element = await _getClassForCodeString('UnknownCtorParamType');

expect(
_generator.generate(element, null),
throwsInvalidGenerationSourceError(
'At least one constructor argument has an invalid type: `number`.',
'Check names and imports.'));
});

test('in fields', () async {
var element = await _getClassForCodeString('UnknownFieldType');

expect(
_generator.generate(element, null),
throwsInvalidGenerationSourceError(
'At least one field has an invalid type: `number`.',
'Check names and imports.'));
});
});

Expand Down Expand Up @@ -126,14 +150,14 @@ Future<Element> _getClassForCodeString(String name) async {
}

Future<CompilationUnit> _getCompilationUnitForString(String projectPath) async {
Source source = new StringSource(_testSource, 'test content');
var source = new StringSource(_testSource, 'test content');

var foundFiles = await getDartFiles(projectPath,
searchList: [p.join(getPackagePath(), 'test', 'test_files')]);

var context = await getAnalysisContextForProjectPath(projectPath, foundFiles);

LibraryElement libElement = context.computeLibraryElement(source);
var libElement = context.computeLibraryElement(source);
return context.resolveCompilationUnit(source, libElement);
}

Expand Down Expand Up @@ -207,4 +231,16 @@ class ParentObjectWithDynamicChildren {
String str;
List<dynamic> children;
}

@JsonSerializable()
class UnknownCtorParamType {
int number

UnknownCtorParamType(Bob number) : this.number = number;
}

@JsonSerializable()
class UnknownFieldType {
Bob number
}
''';
21 changes: 13 additions & 8 deletions test/test_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,22 @@ String getPackagePath() {
return _packagePathCache;
}

final Matcher throwsInvalidGenerationSourceError =
throwsA(isInvalidGenerationSourceError);
Matcher throwsInvalidGenerationSourceError(messageMatcher, todoMatcher) =>
throwsA(allOf(
new isInstanceOf<InvalidGenerationSourceError>(),
new FeatureMatcher<InvalidGenerationSourceError>(
'todo', (e) => e.todo, todoMatcher),
new FeatureMatcher<InvalidGenerationSourceError>(
'message', (e) => e.message, messageMatcher)));

const Matcher isInvalidGenerationSourceError =
const _InvalidGenerationSourceError();
// TODO(kevmoo) add this to pkg/matcher – is nice!
class FeatureMatcher<T> extends CustomMatcher {
final dynamic Function(T value) _feature;

class _InvalidGenerationSourceError extends TypeMatcher {
const _InvalidGenerationSourceError() : super("InvalidGenerationSourceError");
FeatureMatcher(String name, this._feature, matcher)
: super("`$name`", "`$name`", matcher);

@override
bool matches(item, Map matchState) => item is InvalidGenerationSourceError;
featureValueOf(covariant T actual) => _feature(actual);
}

/// Returns all of the declarations in [unit], including [unit] as the first
Expand Down