[Experimental] First attempt at replacing AST traversal with Maybe<T> #180

ghsyeung · 2016-12-07T02:30:27Z

Prototyping of Maybe monad in AST traversal

This is intended to further discuss whether/how Maybe monad would be a good fit in

writing internal AST traversal
jQuery style helpers for Rules development

src/angular/metadataReader.ts seems to be a good target to start. Most of the work goes into extracting AST traversal code into astQuery and ngQuery.

A few realizations so far

Maybe monad introduces a bit a learning curve, but the structure makes the intent easier to understand (e.g. decoratorArgument)
There are a few wrinkles getting the proper type returned, but type guard helps (e.g. callExpression)

mgechev · 2016-12-07T12:52:49Z

Looks hot, happy to see your suggestion in action! Will take a look at it in the next a couple of days and give you feedback. Thanks!

ghsyeung · 2016-12-07T19:38:20Z

Had some more thoughts about refactor experience so far.

Can codelyzer develop against a different TS than the runtime TS? I have a feeling you can, but if not, it harder to write compatible type-friendly code as a lot of useful features like 'control flow based type analysis' and 'dot name type guards' landed only in TS 2.0

More refactoring can be done in src/angular/metadataReader.ts, but that likely require adding a 'seam' to urlResolver and fileResolver to make it Maybe friendly.

Also there may be a few minor bugs, I'll highlight them below.

ghsyeung · 2016-12-07T19:42:33Z

src/util/astQuery.ts

+}
+
+export function isSimpleTemplateString(expr: ts.Expression): expr is (ts.StringLiteral | ts.NoSubstitutionTemplateLiteral) {
+    return expr && expr.kind === kinds.StringLiteral || expr.kind === kinds.NoSubstitutionTemplateLiteral;


I believe https://github.com/mgechev/codelyzer/blob/master/src/util/utils.ts#L29 should use kinds.NoSubstitutionTemplateLiteral instead.

I was using astExplorer and find it odd that it maps to FirstTemplateToken as well.

ghsyeung · 2016-12-07T19:44:13Z

test/angular/metadataReader.spec.ts

-        return url;
-      };
-      const code = `
+      try {


I think we need to wrap with try/finally or the spy would leak when a test fails.

ghsyeung · 2016-12-07T19:46:15Z

src/angular/metadataReader.ts

-    const metadata = this.readDirectiveMetadata(d, dec);
-    const result = new ComponentMetadata();
-    result.selector = metadata.selector;
-    result.controller = metadata.controller;


I notice that decorator is not copied to result. Is that intentional?

Seems like an issue.

ghsyeung · 2016-12-12T10:18:24Z

Added Walker builder (a builder approach and a functional approach) in attempt to make Rule definition easier.

mgechev · 2016-12-19T09:49:00Z

src/angular/metadataReader.ts

+
+    read(d: ts.ClassDeclaration): DirectiveMetadata {
+        let componentMetadata = unwrapFirst(
+            (d.decorators || ([] as ts.Decorator[])).map((dec: ts.Decorator) => {


Looks beautiful!

mgechev · 2016-12-19T09:51:36Z

src/angular/metadataReader.ts

+        const inlineStyles_M = getInlineStyle(dec)
+            .fmap(inlineStyles => {
+                return inlineStyles.elements.map((inlineStyle: ts.Expression) => {
+                    if (isSimpleTemplateString(inlineStyle)) {


Maybe we can first apply filter and after that map it, so we can drop the ugly if condition?

mgechev · 2016-12-19T09:58:51Z

src/util/function.ts

+}
+
+export function all<T>(...preds: F1<T, boolean>[]): F1<T, boolean> {
+    return (t: T) => !preds.find(p => !p(t));


Maybe some can do the trick since it returns boolean anyway.

mgechev · 2016-12-19T09:59:00Z

src/util/function.ts

+    return (t: T) => !preds.find(p => !p(t));
+}
+export function any<T>(...preds: F1<T, boolean>[]): F1<T, boolean> {
+    return (t: T) => !!preds.find(p => p(t));


Maybe some can do the trick since it returns boolean anyway.

mgechev · 2016-12-19T10:08:18Z

I left some comments. In general I like the direction that you took! The build is failing:

src/componentClassSuffixRule.ts(15,31): error TS2304: Cannot find name 'suffix'.
src/componentClassSuffixRule.ts(25,32): error TS2334: 'this' cannot be referenced in a static property initializer.
src/componentClassSuffixRule.ts(25,37): error TS2339: Property 'getOptions' does not exist on type 'typeof Rule'.
src/componentClassSuffixRule.ts(25,63): error TS2334: 'this' cannot be referenced in a static property initializer.
src/componentClassSuffixRule.ts(25,68): error TS2339: Property 'getOptions' does not exist on type 'typeof Rule'.
src/componentClassSuffixRule.ts(26,61): error TS2346: Supplied parameters do not match any signature of call target.

ghsyeung · 2016-12-21T10:40:37Z

Nothing new yet, just fixed the above error resulted from merging master.

mgechev · 2016-12-21T10:49:31Z

Awesome! I just published beta.4, we can introduce this new AST traversal mechanism in beta.5.

First attempt at replacing AST traversal with Maybe<T>

d208a91

ghsyeung commented Dec 7, 2016

View reviewed changes

Gary Yeung added 3 commits December 11, 2016 18:17

Using WalkerFactory to create Ng2Walker

247279d

A more functional approach to creating Walker

3e24bb0

Adding NodeValidator to validate any node that matches a SyntaxKind

531b140

mgechev reviewed Dec 19, 2016

View reviewed changes

Merge commit '13b722e' into refactor_maybe

ba0f67f

ghsyeung force-pushed the refactor_maybe branch from 7e88123 to ba0f67f Compare December 21, 2016 10:37

mgechev merged commit fe42a37 into mgechev:master Dec 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Experimental] First attempt at replacing AST traversal with Maybe<T> #180

[Experimental] First attempt at replacing AST traversal with Maybe<T> #180

ghsyeung commented Dec 7, 2016

mgechev commented Dec 7, 2016

ghsyeung commented Dec 7, 2016

ghsyeung Dec 7, 2016

ghsyeung Dec 7, 2016

ghsyeung Dec 7, 2016

mgechev Dec 19, 2016

ghsyeung commented Dec 12, 2016

mgechev Dec 19, 2016

mgechev Dec 19, 2016

mgechev Dec 19, 2016

mgechev Dec 19, 2016

mgechev commented Dec 19, 2016 •

edited

Loading

ghsyeung commented Dec 21, 2016

mgechev commented Dec 21, 2016

[Experimental] First attempt at replacing AST traversal with Maybe<T> #180

[Experimental] First attempt at replacing AST traversal with Maybe<T> #180

Conversation

ghsyeung commented Dec 7, 2016

mgechev commented Dec 7, 2016

ghsyeung commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghsyeung commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgechev commented Dec 19, 2016 • edited Loading

ghsyeung commented Dec 21, 2016

mgechev commented Dec 21, 2016

mgechev commented Dec 19, 2016 •

edited

Loading