Summary integration by mk13 · Pull Request #276 · dart-archive/angular_analyzer_plugin

mk13 · 2017-04-11T23:04:21Z

One more try.

…nalyzer

MichaelRFairhurst

I think I do see one bug. In lib/tasks.dart, we have a list of errors that turn into a map of codes to types, which is used to deserialize errors from summaries.

We'll likely need a similar pattern for ast errors.

I bet that if you open up green tea, then hit reanalyze, all the syntactic errors will disappear because they will fail to be deserialized without a means to go from string error code to object error code.

Really looking forward to getting this merged. Love the new parser.

MichaelRFairhurst · 2017-04-11T23:14:33Z

-import 'package:html/parser.dart' as html;
-import 'package:source_span/source_span.dart';
-
-html.Element firstElement(html.Node node) {


Nice we can remove this :)

MichaelRFairhurst · 2017-04-11T23:14:54Z


+  // Following Angular2 Logic:
+  // https://github.com/dart-lang/angular2/blob/8220ba3a693aff51eed33cd1ec9542bde9017423/lib/src/compiler/schema/dom_element_schema_registry.dart#L199
+  static const attrToPropMap = const {


should this be done in angular_ast?

One problem I see with this is that angular_ast doesn't keep track of two different names, which could make the offset off for 'class':'className' conversion. ExpressionBoundAttribute class keeps track of two different names: origName and propName, which allows us some flexibility in what is being interpreted for value and one for offset.

From our conversation -- agreed. This is the right place for this!

MichaelRFairhurst · 2017-04-11T23:15:13Z

+    return root;
+  }
+
+  NodeInfo convert(StandaloneTemplateAst node, {ElementInfo parent}) {


parent is no longer optional, really, right?

You could make it just a second parameter, or to keep the parent: naming (kinda nice right?) you can mark it @required which will make it an error if we forget to use parent: even though its a named (as opposed to positional) param.

MichaelRFairhurst · 2017-04-11T23:15:31Z

+      } else {
+        openingSpan = _toSourceRange(
+            node.beginToken.offset, node.endToken.end - node.beginToken.offset);
+        openingNameSpan =


there's no real opening/closing name span in angular ast? Would be nice to not make it openingNameSpan - '<'.length, but something tracked at parse time.

I can use the identifierToken actually so it will be similar to how openingSpan is defined.
openingNameSpan = _toSourceRange(identifierToken.offset, identifierToken.length)

MichaelRFairhurst · 2017-04-11T23:15:54Z

+            node.beginToken.offset, node.endToken.end - node.beginToken.offset);
+        openingNameSpan =
+            new SourceRange(openingSpan.offset + '<'.length, localName.length);
+        var pnode = node as ParsedEmbeddedContentAst;


Why is this cast necessary? Can't the Parsed and Synthetic versions of EmbeddedContentAst both share the same interface?

Parsed version have access to tokens, synthetic does not and the tokens contain the direct offsets. As a result, the casting is necessary.

Interesting. Ok, just wanted to ask!

In fact that's great, as compared to getting null unexpectedly.

MichaelRFairhurst · 2017-04-11T23:22:36Z

-            template.view.classElement, dartRequest,
-            skipChildClass: false));
+        var memberContributor = new TypeMemberContributor();
+        var inheritedContributor = new InheritedReferenceContributor();


Liking the reduction of types that don't help readability. Very nice! Though I might have preferred final, not everyone does, so it can be a controversy worth avoiding :) So looks great!

MichaelRFairhurst · 2017-04-11T23:22:51Z

+          inheritedContributor.computeSuggestionsForClass(
+            template.view.classElement,
+            dartRequest,
+            skipChildClass: false,


dangling comma?

Ah turns out if you put a dangling comma at the end and call 'dartfmt', it makes long lists very neatly aligned. If it's a long list with nested functions, I think this adds to readability in exchange for some additional lines.

MichaelRFairhurst · 2017-04-11T23:22:55Z

-                suggestions, varExtractor.variables, dartRequest.opType);
+              suggestions,
+              varExtractor.variables,
+              dartRequest.opType,


MichaelRFairhurst · 2017-04-11T23:23:11Z

+              suggestTransclusions(target.parent, suggestions);
+            }
+          }
+          // Directly within closing tag; suggest nothing. Ex: '</div^>'


Once again, nice comments.

Can you maybe move this comment inside the else block? Otherwise I read this as the end of the control structure, takes me a moment to realize that the "else" goes here.

MichaelRFairhurst · 2017-04-11T23:23:17Z

+            }
+          }
+          // Directly within closing tag; suggest nothing. Ex: '</div^>'
+          else


MichaelRFairhurst

Nice changes so far, looking great

MichaelRFairhurst · 2017-04-14T15:36:54Z

+            valueOffset, dartParser.findMustaches(value, valueOffset)));
+      }
+    }
+    ;


dangling semi

MichaelRFairhurst · 2017-04-14T15:37:05Z

+            property, "[", "]", ExpressionBoundType.input));
+      }
+    }
+    ;


MichaelRFairhurst · 2017-04-14T15:37:25Z

+            valueOffset, dartParser.findMustaches(value, valueOffset)));
+      }
    }
+    ;


dangling semi

MichaelRFairhurst · 2017-04-14T15:37:33Z

+            property, "[", "]", ExpressionBoundType.input));
+      }
+    }
+    ;


MichaelRFairhurst · 2017-04-14T15:37:41Z

+          valueOffset,
+          dartParser.findMustaches(value, valueOffset)));
+    }
+    ;


MichaelRFairhurst · 2017-04-14T15:39:11Z

+  void assertMultipleErrorsExplicit(
+    Source source,
+    String code,
+    List<Tuple4<String, int, ErrorCode, List<Object>>> expectedErrors,


MichaelRFairhurst · 2017-04-14T15:41:33Z

+   *    code segment where offset begins,
+   *    length of the error highlight,
+   *    errorCode,
+   *    and optional error args - pass empty list if not needed.


only just realized that your new assertion function here checks the args. I've wanted to have tests for them, this is great!

MichaelRFairhurst · 2017-04-14T15:43:06Z

-          NgParserWarningCode.EXPECTED_WHITESPACE_BEFORE_NEW_DECORATOR),
-      new AnalysisError(htmlSource, 6, 14, NgParserWarningCode.SUFFIX_PROPERTY),
+    assertMultipleErrorsExplicit(htmlSource, code, [
+      new Tuple4(']', 0, AngularWarningCode.NONEXIST_INPUT_BOUND, ['']),


Hmm, hadn't noticed before that these errors had a length of zero. How does intelliJ handle that?

When you double click it it puts the cursor right at the spot, but doesn't squiggle anything since it's of length 0. I'm planning sometime later once the ast is merged to do another pass and make the error messages more helpful in terms of length.

MichaelRFairhurst · 2017-04-14T15:43:51Z

-    expect(realErrors.length, expectedErrors.length,
-        reason: 'Expected error counts do not  match.');
+    for (Tuple4 expectedError in expectedErrors) {
+      var offset = code.indexOf(expectedError.item1);


Might want to assert that this isn't -1, or maybe even assert that the expected code doesn't occur more than once in the test file?

Looks like you did this, and github didn't realize. Commenting here so we know

MichaelRFairhurst · 2017-04-14T15:45:13Z

+              suggestTransclusions(target.parent, suggestions);
+            }
+          }
+          // Directly within closing tag; suggest nothing. Ex: '</div^>'


Can you maybe move this comment inside the else block? Otherwise I read this as the end of the control structure, takes me a moment to realize that the "else" goes here.

…te type.

mk13 · 2017-04-15T00:12:43Z

Still need to add change such that conversion does not have three semi-redundant conversions based on check of ElementAst, EmbeddedTemplateAst, and EmbeddedContentAst.

Need to add some refactor into angular_ast to create a generic encapsulation interface and want to re-factor EmbeddedContentAst generation to recycle some code instead of creating its own (seemingly wasteful) logic.

MichaelRFairhurst

Nice changes

MichaelRFairhurst · 2017-04-17T17:26:49Z

+    List<ParsedEventAst> events: const [],
+    List<ParsedPropertyAst> properties: const [],
+    List<ParsedReferenceAst> references: const [],
+    List<ParsedStarAst> stars: const [],


I like the use of named params, makes it really clean when called

(there should probably be a lint for having more than x unnamed parameters, don't you think?)

Agreed, especially when the values have defaults - makes things so much cleaner.

MichaelRFairhurst · 2017-04-17T17:28:12Z


-    element.events.forEach((event) {
-      attributes.add(_convertStatementsBoundAttribute(event, "(", ")"));
+    bananas.forEach((banana) {


this could -- depending on preference -- be

bananas.map(_convertExpressionBoundAttribute).forEach(returnAttributes.add)

I like it, changing it 👍

MichaelRFairhurst · 2017-04-17T17:29:57Z

-
-      value = ast.value;
+      ParsedEventAst ast) {
+    var origName =


nested ternaries...hmm, should probably have at least one if statement I think

MichaelRFairhurst · 2017-04-17T17:34:52Z

-    expect(realErrors.length, expectedErrors.length,
-        reason: 'Expected error counts do not  match.');
+    for (Tuple4 expectedError in expectedErrors) {
+      var offset = code.indexOf(expectedError.item1);


Looks like you did this, and github didn't realize. Commenting here so we know

MichaelRFairhurst · 2017-04-17T17:35:17Z

    git:
      url: https://github.com/mk13/angular_ast
-      ref: master
+      ref: on_bind_parsing


This can be master again, right?

Oh yeah, that's true. But I might have to switch this again.

mk13 · 2017-04-18T18:41:48Z

I wanted to condense the three element type condensing - but that depends on the condensing of the three types in the angular_ast (dart-archive/angular_ast#34). Seeing as how it needs to be discussed more, it might be a good point to end this PR here and make the condensing of Element types into a separate PR.

Ending the refactor of code/code clean-up and moving onto resolving the potential issue of errors not properly deserializing.

…mplate> causing crash

mk13 · 2017-04-18T22:56:22Z

Ping @MichaelRFairhurst for approval.

Modified logic that loads the error codes into memory. Think we should add this logic here? or create a separate map within angular_ast?

MichaelRFairhurst · 2017-04-19T18:40:24Z

Definitely worth building and running against greentea as a last check, but it LGTM!

@Attribute

* Summary integration (#276) * Sync to origin master * checkpoint * Initial ast integration - ranges not behaving properly * Minor bug fix * checkpoint * Tasks tests all passing * Checkpoint before error code fix * Tasks tests set back to 100% after introducing error codes * resolver_test midway checkpoint * Checkpoint * Full test coverage in analyzer. Server still left * All tests passing * Make paths explicit * Remove comments * Attributes now sorted by offset. Improves performance in auto completion * DOCTYPE now acceptable * Created separate Ast for top level document container * Refactored primary function in completion.dart * Minor code clean up * Fuzz tester bug fix * Revert try catch block * Rename exception codes after changes in angular_ast * Bug fix on [class. [style. [attr. bindings; range error * Allow spaces in msutaches * Autocompletion in text attribute value is now empty * Allow for plain [class] binding * Fixed update_deps.sh to work with angular_ast * Clean up deps and remove some unnecessary comments * Changed pubspec to pull from master * Defend against files hidden by generated files (#271) * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * Travis sdk fix (#272) * Sync to origin master * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * First round of quick fixes based on feedback * remove comment * Error codes no longer use hard offsets; uses string instead * Merged attribute generating logic to be agnostic of Element or template type. * Assert for indexOf added into test. Moved comments for clarity * Remove comment-blocked chunk * Changes based on feedback * Error codes added to hashmap so now show up. Fixed issue with <div template> causing crash * Changed pubspec to pull from master in angular_ast * Summary integration sync (#288) * Sync to origin master * checkpoint * Initial ast integration - ranges not behaving properly * Minor bug fix * checkpoint * Tasks tests all passing * Checkpoint before error code fix * Tasks tests set back to 100% after introducing error codes * resolver_test midway checkpoint * Checkpoint * Full test coverage in analyzer. Server still left * All tests passing * Make paths explicit * Remove comments * Attributes now sorted by offset. Improves performance in auto completion * DOCTYPE now acceptable * Created separate Ast for top level document container * Refactored primary function in completion.dart * Minor code clean up * Fuzz tester bug fix * Revert try catch block * Rename exception codes after changes in angular_ast * Bug fix on [class. [style. [attr. bindings; range error * Allow spaces in msutaches * Autocompletion in text attribute value is now empty * Allow for plain [class] binding * Fixed update_deps.sh to work with angular_ast * Clean up deps and remove some unnecessary comments * Changed pubspec to pull from master * Defend against files hidden by generated files (#271) * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * Travis sdk fix (#272) * Sync to origin master * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * First round of quick fixes based on feedback * Tick to 8 (#277) * Sync to origin master * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * Tick to 0.0.8 * remove comment * Error codes no longer use hard offsets; uses string instead * Track @Attribute annotated constructor parameters (#270) * Merged attribute generating logic to be agnostic of Element or template type. * Assert for indexOf added into test. Moved comments for clarity * Remove comment-blocked chunk * Resolve plain attributes -- in ranges, and strchecks for inputs. (#278) * Resolve plain attributes -- in ranges, and strchecks for inputs. Record resolved ranges for `x="y"` where `x` is an input or a constructor `@Attribute`. If its an input, check that strings are assignable to it. If not, give it its own error that hopefully explains the semi magical situation. * Track/typecheck std attrs Eventually we should * report errors for <a href="foo" [href]="bar"> * not suggest href after [href] has been used * not suggest [href] after href has been used * Changes based on feedback * Error codes added to hashmap so now show up. Fixed issue with <div template> causing crash * Changed pubspec to pull from master in angular_ast * Template parsing fix (#279) * Sync to origin master * Fix travis build. Fix sdk related errors (ContextRoot and IdeOptions). Clean up imports to remove unused * Fix issue that caused empty binding on template attribute to crash * Now ready: Priority angular analysis (#273) * Priority dart & html analysis * Starter class, await supplementing errors or they're added too late. * Report has work to analyze for priority requests * Don't await for requesting files, do have a cache busting option * Handle case of files being requested multiple times * Priority html requests, requires serving errors with line infos. * Report all html files for all dart contexts in one method, plus tests I think before we were getting a last-one-wins effect, where the last analyzed dart/html pair would cover up all the errors for every other dart/html pair. As such, I need to carefully test that this change doesn't introduce new false errors (from them being covered up before). But now with this, we can report html errors back for multiple contexts from a single method when that's requested. Next I just have to use that method on getErrors when we have multiple relationships tracked already. * Guess html/dart relationship when none exist, otherwise use what's known using isEmpty rather than == 0 * Get setterType by setter.parameters[0] instead of setter.variable.type (#283) Seems like in the precense of a getter, setter.variable.type is the getter type, not the setter's type. So use the first function argument instead. * Remove contextRoot, won't be required in master soon, and breaks dev (#284) * Disable global attr plaintext typechecking until #280 has a clear solution (#286) * Update test_reflective_loader, fix analyzer warnings&errors (#285) * Update test_reflective_loader, fix analyzer warnings&errors * Fix the single server test too, which prevents build errors * Use var instead of types in angular_driver_test * Add TODOs * Percentage use fix in style.width and style.height. Test cases added (#282) * Percentage use fix in style.width and style.height. Test cases added * Added more property names that use percentage * Tests passing * Sync to master; tests passing

mk13 added 30 commits November 30, 2016 10:41

Sync to origin master

41458dc

Merge with origin master

6395e37

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

8eedfce

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

1eba56f

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

2e0f1a4

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

9881a95

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

d46b2de

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

6489912

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

c453117

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

dd5fee4

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

ca77fc4

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

b0e3635

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

cdcc5ff

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

1c04147

…nalyzer

Merge branch 'master' of https://github.com/dart-lang/angular2-dart-a…

11de100

…nalyzer

checkpoint

d39f620

Initial ast integration - ranges not behaving properly

bea4a90

Minor bug fix

f8494d1

checkpoint

4911ffe

Tasks tests all passing

f1f19a0

Checkpoint before error code fix

df8ddc0

Tasks tests set back to 100% after introducing error codes

ff0be32

resolver_test midway checkpoint

390c09d

Checkpoint

9319791

Full test coverage in analyzer. Server still left

11a613a

All tests passing

743b41b

Make paths explicit

1e71856

Remove comments

ef23f0e

Attributes now sorted by offset. Improves performance in auto completion

c0ac16e

DOCTYPE now acceptable

5decb80

mk13 removed the cla: no label Apr 11, 2017

mk13 requested a review from MichaelRFairhurst April 11, 2017 23:11

MichaelRFairhurst suggested changes Apr 11, 2017

View reviewed changes

First round of quick fixes based on feedback

bf5b69f

googlebot added cla: no and removed cla: yes labels Apr 12, 2017

mk13 added 2 commits April 13, 2017 10:27

remove comment

390d63a

Error codes no longer use hard offsets; uses string instead

4998c16

MichaelRFairhurst reviewed Apr 14, 2017

View reviewed changes

mk13 added 2 commits April 14, 2017 17:03

Merged attribute generating logic to be agnostic of Element or templa…

c81ef23

…te type.

Assert for indexOf added into test. Moved comments for clarity

f930897

Remove comment-blocked chunk

12d6d6a

MichaelRFairhurst reviewed Apr 17, 2017

View reviewed changes

Changes based on feedback

e1c2949

mk13 added cla: yes and removed cla: no labels Apr 18, 2017

googlebot added cla: no and removed cla: yes labels Apr 18, 2017

Error codes added to hashmap so now show up. Fixed issue with <div te…

989b3b9

…mplate> causing crash

mk13 mentioned this pull request Apr 18, 2017

List error codes dart-archive/angular_ast#35

Merged

Changed pubspec to pull from master in angular_ast

6be99fb

MichaelRFairhurst approved these changes Apr 19, 2017

View reviewed changes

mk13 added cla: yes and removed cla: no labels Apr 20, 2017

mk13 merged commit 4ac4248 into dart-archive:angular_ast_integration Apr 20, 2017

Uh oh!

Conversation

mk13 commented Apr 11, 2017

Uh oh!

MichaelRFairhurst left a comment

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!

mk13 Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MichaelRFairhurst left a comment

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!

mk13 commented Apr 15, 2017

Uh oh!

MichaelRFairhurst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mk13 Apr 12, 2017 •

edited

Loading

mk13 commented Apr 18, 2017 •

edited

Loading

mk13 commented Apr 18, 2017 •

edited

Loading