-
Notifications
You must be signed in to change notification settings - Fork 66
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
Rewrite: 1.0.0-alpha, with full statement support, fluid syntax #15
Conversation
clazz.addMethod( | ||
new MethodBuilder.returnVoid( | ||
'eat', | ||
returns: reference('print).call([literal('Yum')]), |
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.
missing closing '
after print
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
/// // Outputs: new Future.value('Hello') | ||
/// async.Future.newInstanceNamed('value', [literal('Hello')]); | ||
/// | ||
/// If you are [missing a reference from `dart:async`](https://goo.gl/XbSfmT) |
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.
"missing a symbol from"?
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
import 'package:code_builder/code_builder.dart'; | ||
|
||
/// A namespace for references in `dart:async`. | ||
final DartAsync async = new DartAsync._(); |
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.
async
is a Dart keyword. How about $async
or libasync
?
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
|
||
import 'package:code_builder/code_builder.dart'; | ||
|
||
/// A namespace for references in `dart:async`. |
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.
nit: references to?
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
|
||
/// Clones all annotations to [clone]. | ||
void cloneAnnotationsTo(HasAnnotations clone) { | ||
clone.addAnnotations(_annotations); |
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.
The AnnotationBuilder
s do not seem to be cloned. After this operation both the clone
and this
will refer to the same builder objects.
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.
Renamed as "copyTo"
} | ||
|
||
/// Wrap [member] to be emitted as a `static` method or field. | ||
_StaticFieldWrapper asStatic(ValidClassMember member) { |
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.
Public function returning private type feels wrong. Can it return ValidClassMember
? Or can _StatisFieldWrapper
be made public?
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 (former)
: _asAbstract = asAbstract, | ||
_extends = asExtends, | ||
_with = asWith.toList(), | ||
_implements = asImplements.toList(); |
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.
nit: I think this would read a bit better like this: _implements = asImplements ?? <TypeBuilder>[];
. The default value would be null
.
(applies to asWith
too)
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.
Big difference here is I am purposefully making a defensive copy.
/// 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> { |
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.
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?
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.
Not too crazy. Maybe too crazy for 1.0 though :)
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. |
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'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 :)
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.
lazily done
# Directory created by dartdoc | ||
doc/api/ | ||
|
||
# Don't commit pubspec lock file | ||
# (Library packages only! Remove pattern if developing an application package) | ||
pubspec.lock | ||
|
||
*.iml |
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.
why get rid of this?
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.
There's this issue to watch out for: flutter/flutter-intellij#302
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 was advised to just use a global gitignore for my IDEs
show ifThen, elseIf, elseThen, IfStatementBuilder, StatementBuilder; | ||
export 'src/builders/type.dart' show NewInstanceBuilder, TypeBuilder; | ||
|
||
final _dartFmt = new DartFormatter(); |
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.
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
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
// 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. |
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.
reference -> references
here and every other dart/*.dart library 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.
done
} | ||
|
||
@override | ||
ExpressionBuilder negate() => new _NegateExpression(this); |
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.
also override operator unary- ?
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
} | ||
|
||
/// Implements much of [ExpressionBuilder]. | ||
abstract class AbstractExpressionMixin implements ExpressionBuilder { |
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.
We also need null-aware assignment
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
); | ||
} | ||
|
||
/// Short-hand for [FieldBuilder]. |
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.
comment for this and varConst
should be 'shorthand for Fieldbuilder.asFinal' or 'asConst'
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
import 'package:code_builder/src/builders/type.dart'; | ||
import 'package:code_builder/src/tokens.dart'; | ||
|
||
/// A more short-hand way of constructing a [ParameterBuilder]. |
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.
get rid of "more", it's cleaner
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
} | ||
|
||
/// Invokes `const` on this type with a [name]d constructor. | ||
NewInstanceBuilder constInstanceWith( |
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.
constInstanceWith -> namedConstInstance?
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
/// // Prints Identifier "i1.Bar" | ||
/// print(scope.getIdentifier('Bar', 'package:foo/foo.dart'); | ||
/// // May print 'i1.Bar'. | ||
/// print(scope.identifier('Bar'), 'package:foo/foo.dart'); |
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.
here and in the Baz
example, the closing parenthesis should be after the package, not the identifier
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
factory Scope.dedupe() = _DeduplicatingScope; | ||
/// Create a new scoping context. | ||
/// | ||
/// Actual implementation of is _not_ guaranteed, only that all import |
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.
Actual implementation of what?
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.
Clarified
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. .
@googlebot: This was me :-( |
CLAs look good, thanks! |
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:
/cc @yjbanov @hterkelsen