fix(StaticMetadataExtractor): Map members annotations to all annotations #907
Conversation
| ).then((_) { | ||
| controller.close(); | ||
| gatherReferencedUris(transform, resolver, options, templatesOnly: true) | ||
| .then((templates) { |
There was a problem hiding this comment.
Had to ask @munificent on this one, his answer:
- function bodies are always indented 2 spaces from the previous line.
- closing bracket of closures always line up from preceeding line.
so it'd look like:
gatherReferencedUris(transform, resolver, options, templatesOnly: true)
.then((templates) {
templates.values...;
}).then((_) {
return _findHtmlEntry..;
}).then((htmlRefId) {
});There was a problem hiding this comment.
I'll change for to conform to your points (especially the second one).
Not sure it will look like your snippet though (3rd line) ;)
There was a problem hiding this comment.
Yeah, we mentioned that the style guide really needs this type of example as it comes up a lot.
The indent by 4 for the first .then line followed by 2 for the function body feels a bit weird, but the answer was 'consistency, and you'll get used to it'. But I'll admit that I haven't settled on any particular syntax. I think the portion that weirded me out was the closing brackets on the same line :)
|
@blois I think all you concerns so far have been addressed. Let me know if you have more. Thanks for the feedback. |
| @@ -11,8 +11,8 @@ class ContainsSelector { | |||
| } | |||
| } | |||
|
|
|||
| RegExp _SELECTOR_REGEXP = new RegExp(r'^(?:([\w\-]+)|(?:\.([\w\-]+))|(?:\[([\w\-\*]+)(?:=([^\]]*))?\]))'); | |||
| RegExp _COMMENT_COMPONENT_REGEXP = new RegExp(r'^\[([\w\-]+)(?:\=(.*))?\]$'); | |||
| RegExp _SELECTOR_REGEXP = new RegExp(r'^(?:([-\w]+)|(?:\.([-\w]+))|(?:\[([-\w*]+)(?:=([^\]]*))?\]))'); | |||
There was a problem hiding this comment.
Can these regexp changes be split into a separate PR?
|
The question: "why do we need to do fancy stuff (check if the class has a map member, assignable to a Dart map, ...) instead of looking only at the base class ?" It allows any subclasses of AbstractNgAnnotation- including custom subclasses. So someone could add a subclass which does not have a constructor which takes the named map parameter, in which case this would generate incorrect code. Even with the checks it's a bit hokey given that we're modifying the arguments to a constructor which we know very little about. But it tries to do it's best, since if it generates bad syntax it's not always clear where that syntax originated from. |
| .then(controller.add) | ||
| .catchError((e) { | ||
| transform.logger.warning('Unable to find $id from ' | ||
| t'html_files in pubspec.yaml.'); |
There was a problem hiding this comment.
And an additional space 'html_files in'.
This gets picked up because the unittests are checking various error conditions:
dart --checked test/tools/transformer/all.dart
|
Overall I think it looks OK- a few nits introduced with the formatting changes, but the primary functionality changes are fine. |
|
Please either hold of submitting this PR until after #908 or revert all conflicting cleanup. |
|
#926 contains only the useful parts (drop style changes) |
@blois I would appreciate if you can review this.
The changes are in the second commit. The first one is only about bringing some consistency in code style vs the rest of a.dart.
One question for you: why do we need to do fancy stuff (check if the class has a map member, assignable to a Dart map, ...) instead of looking only at the base class ?
I'll rebase #861 on top of this when this (eventually) gets merged.