-
Notifications
You must be signed in to change notification settings - Fork 62
More work towards feature parity of 1x #116
Conversation
|
Ping @alorenzen since you're back - this is probably enough to roll I imagine. |
lib/src/allocator.dart
Outdated
| /// | ||
| /// For example, a no-op implementation: | ||
| /// ```dart | ||
| /// allocate(const Reference('List', 'dart:core')); // Outputs 'List'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: I'd consider replacing Outputs 'List' with Returns 'List'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/src/allocator.dart
Outdated
| /// | ||
| /// Where-as an implementation that prefixes imports might output: | ||
| /// ```dart | ||
| /// allocate(const Reference('Foo', 'package:foo')); // Outputs '_i1.Foo'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, consider replacing Outputs with Returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| class DartEmitter extends GeneralizingSpecVisitor<StringSink> { | ||
| const DartEmitter(); | ||
| class DartEmitter implements SpecVisitor<StringSink> { | ||
| final Allocator _allocator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think _allocator.imports is ever called. So how would you actually print out the imports in a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch. Added to visitFile and updated file_test.
| test('should emit a source file', () { | ||
| expect( | ||
| new File((b) => b | ||
| ..directives.add(new Directive.import('dart:collection')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assumption that you always have to add the imports manually? Can you add a test which uses allocator.imports instead?
Also, maybe a test case for prefixing imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Added test cases.
matanlurey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL.
lib/src/allocator.dart
Outdated
| /// | ||
| /// For example, a no-op implementation: | ||
| /// ```dart | ||
| /// allocate(const Reference('List', 'dart:core')); // Outputs 'List'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/src/allocator.dart
Outdated
| /// | ||
| /// Where-as an implementation that prefixes imports might output: | ||
| /// ```dart | ||
| /// allocate(const Reference('Foo', 'package:foo')); // Outputs '_i1.Foo'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| class DartEmitter extends GeneralizingSpecVisitor<StringSink> { | ||
| const DartEmitter(); | ||
| class DartEmitter implements SpecVisitor<StringSink> { | ||
| final Allocator _allocator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch. Added to visitFile and updated file_test.
| test('should emit a source file', () { | ||
| expect( | ||
| new File((b) => b | ||
| ..directives.add(new Directive.import('dart:collection')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Added test cases.
* WIP. * Finish up feature parity. * Dartfmt. * Address feedback. * Format with dart_style:format for now. * Fix tests.
* WIP. * Finish up feature parity. * Dartfmt. * Address feedback. * Format with dart_style:format for now. * Fix tests.
Added:
Allocator(prefixing)DirectiveandFile