Rewrite: 1.0.0-alpha, with full statement support, fluid syntax #15

matanlurey · 2016-10-16T22:41:14Z

This is a large refactor of of code_builder to able to replace the Angular Output AST code.

Still missing before PR ready for review:

Re-add scoping support
More e2e-style tests that existed in v0 branch

yjbanov · 2016-10-24T16:26:17Z

README.md

+clazz.addMethod(
+  new MethodBuilder.returnVoid(
+    'eat',
+    returns: reference('print).call([literal('Yum')]),


missing closing ' after print

yjbanov · 2016-10-24T16:41:26Z

lib/dart/async.dart

+///     // Outputs: new Future.value('Hello')
+///     async.Future.newInstanceNamed('value', [literal('Hello')]);
+///
+/// If you are [missing a reference from `dart:async`](https://goo.gl/XbSfmT)


"missing a symbol from"?

yjbanov · 2016-10-24T16:42:50Z

lib/dart/async.dart

+import 'package:code_builder/code_builder.dart';
+
+/// A namespace for references in `dart:async`.
+final DartAsync async = new DartAsync._();


async is a Dart keyword. How about $async or libasync?

yjbanov · 2016-10-24T16:44:50Z

lib/dart/async.dart

+
+import 'package:code_builder/code_builder.dart';
+
+/// A namespace for references in `dart:async`.


nit: references to?

yjbanov · 2016-10-24T16:49:02Z

lib/src/builders/annotation.dart

+
+  /// Clones all annotations to [clone].
+  void cloneAnnotationsTo(HasAnnotations clone) {
+    clone.addAnnotations(_annotations);


The AnnotationBuilders do not seem to be cloned. After this operation both the clone and this will refer to the same builder objects.

Renamed as "copyTo"

yjbanov · 2016-10-24T16:50:48Z

lib/src/builders/class.dart

+}
+
+/// Wrap [member] to be emitted as a `static` method or field.
+_StaticFieldWrapper asStatic(ValidClassMember member) {


Public function returning private type feels wrong. Can it return ValidClassMember? Or can _StatisFieldWrapper be made public?

done (former)

yjbanov · 2016-10-24T16:55:19Z

lib/src/builders/class.dart

+      : _asAbstract = asAbstract,
+        _extends = asExtends,
+        _with = asWith.toList(),
+        _implements = asImplements.toList();


nit: I think this would read a bit better like this: _implements = asImplements ?? <TypeBuilder>[];. The default value would be null.

(applies to asWith too)

Big difference here is I am purposefully making a defensive copy.

yjbanov · 2016-10-24T16:59:34Z

lib/src/builders/shared.dart

+/// Most builders should also have specific typed methods for returning their
+/// type of AST node, such as `buildExpression` for returning an `Expression`
+/// AST.
+abstract class AstBuilder<T extends AstNode> {


Crazy idea. What if AstBuilder was extended rather than implemented? Then, in checked mode the AstBuilder constructor could capture the source code location where the AST node was emitted by throwing that catching a stacktrace then walking up the stack until you find that stack frame outside package:code_builder. Too crazy?

Not too crazy. Maybe too crazy for 1.0 though :)

yjbanov · 2016-10-24T19:42:15Z

lib/src/builders/annotation.dart

+import 'package:code_builder/src/builders/parameter.dart';
+import 'package:code_builder/src/builders/shared.dart';
+
+/// Lazily builds an [Annotation] AST when [buildAnnotation] is invoked.


I'd remove "Lazily" from the comments throughout the package. This word only makes me wonder what it means and might scare me away from using this class :)

lazily done

harryterkelsen · 2016-10-21T17:12:10Z

.gitignore

 # (Library packages only! Remove pattern if developing an application package)
 pubspec.lock
-
-*.iml


why get rid of this?

There's this issue to watch out for: flutter/flutter-intellij#302

I was advised to just use a global gitignore for my IDEs

harryterkelsen · 2016-10-21T19:58:28Z

lib/code_builder.dart

+    show ifThen, elseIf, elseThen, IfStatementBuilder, StatementBuilder;
+export 'src/builders/type.dart' show NewInstanceBuilder, TypeBuilder;
+
+final _dartFmt = new DartFormatter();


Maybe move this and dartfmt to pretty_printer? Seems a little out of place here. This library would work better as a simple exporter of implementations

harryterkelsen · 2016-10-21T20:00:20Z

lib/dart/async.dart

+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Contains reference to the `dart:async` SDK for use in code generation.


reference -> references
here and every other dart/*.dart library comment

harryterkelsen · 2016-10-21T22:46:43Z

lib/src/builders/expression.dart

+  }
+
+  @override
+  ExpressionBuilder negate() => new _NegateExpression(this);


also override operator unary- ?

harryterkelsen · 2016-10-21T22:47:55Z

lib/src/builders/expression.dart

+}
+
+/// Implements much of [ExpressionBuilder].
+abstract class AbstractExpressionMixin implements ExpressionBuilder {


We also need null-aware assignment

harryterkelsen · 2016-10-21T23:51:26Z

lib/src/builders/field.dart

+  );
+}
+
+/// Short-hand for [FieldBuilder].


comment for this and varConst should be 'shorthand for Fieldbuilder.asFinal' or 'asConst'

harryterkelsen · 2016-10-25T07:48:54Z

lib/src/builders/parameter.dart

+import 'package:code_builder/src/builders/type.dart';
+import 'package:code_builder/src/tokens.dart';
+
+/// A more short-hand way of constructing a [ParameterBuilder].


get rid of "more", it's cleaner

harryterkelsen · 2016-10-25T07:56:44Z

lib/src/builders/type.dart

+  }
+
+  /// Invokes `const` on this type with a [name]d constructor.
+  NewInstanceBuilder constInstanceWith(


constInstanceWith -> namedConstInstance?

harryterkelsen · 2016-10-25T07:59:28Z

lib/src/scope.dart

-///       // Prints Identifier "i1.Bar"
-///       print(scope.getIdentifier('Bar', 'package:foo/foo.dart');
+///       // May print 'i1.Bar'.
+///       print(scope.identifier('Bar'), 'package:foo/foo.dart');


here and in the Baz example, the closing parenthesis should be after the package, not the identifier

harryterkelsen · 2016-10-25T08:00:18Z

lib/src/scope.dart

-  factory Scope.dedupe() = _DeduplicatingScope;
+  /// Create a new scoping context.
+  ///
+  /// Actual implementation of is _not_ guaranteed, only that all import


Actual implementation of what?

googlebot · 2016-10-27T08:01:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

Address comments and presubmit script. .

matanlurey · 2016-10-27T08:12:56Z

@googlebot: This was me :-(

googlebot · 2016-10-27T08:12:58Z

CLAs look good, thanks!

…-archive/code_builder#15) * Branch to start v1, a partial rewrite. * Incremental work. * Start on new v1 * . * . * . * . * . * . * Another checkpoint. * Another checkpoint with if statements. * Next large incremental work. * Fix docs, more dart core types. * Add scoping back. * Add e2e test. * Update docs, pubspec. * Address comments. Address comments and presubmit script. . * . * .

matanlurey added 11 commits October 9, 2016 16:45

Branch to start v1, a partial rewrite.

5481076

Incremental work.

4fc528f

Start on new v1

c70a7e0

.

9a9b319

.

860ee95

.

627042f

.

0a181a0

.

d808e6f

.

ad989ef

Another checkpoint.

597f672

Another checkpoint with if statements.

daabd51

googlebot added the cla: yes label Oct 16, 2016

matanlurey added 5 commits October 17, 2016 09:00

Next large incremental work.

858bc0c

Fix docs, more dart core types.

4006396

Add scoping back.

e3e7f35

Add e2e test.

92b50f7

Update docs, pubspec.

8b8613f

matanlurey assigned yjbanov and harryterkelsen Oct 20, 2016

yjbanov approved these changes Oct 24, 2016

View reviewed changes

harryterkelsen approved these changes Oct 25, 2016

View reviewed changes

googlebot added cla: no and removed cla: yes labels Oct 27, 2016

Address comments.

cacaa75

Address comments and presubmit script. .

matanlurey force-pushed the v2_dev branch from 7648514 to cacaa75 Compare October 27, 2016 08:05

googlebot added cla: yes and removed cla: no labels Oct 27, 2016

matanlurey added 2 commits October 27, 2016 01:16

.

5f4743f

.

c443ccb

matanlurey merged commit 62d8db1 into dart-archive:master Oct 27, 2016

matanlurey deleted the v2_dev branch October 27, 2016 08:24


		import 'package:code_builder/code_builder.dart';

		/// A namespace for references in `dart:async`.

Rewrite: 1.0.0-alpha, with full statement support, fluid syntax #15

Rewrite: 1.0.0-alpha, with full statement support, fluid syntax #15

Uh oh!

Conversation

matanlurey commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

matanlurey commented Oct 16, 2016 •

edited

Loading