getStructure #346

cancerberoSgx · 2018-06-12T18:18:35Z

I think this is ready:

scripts/ensureForEachStructureClassImplementsGetStructureMethod.ts to ensure for every structure its corresponding node kind implements getStruture method
for each node kind implementing getStructure there is a test (more or less exhaustively)
master merged
most of your review comments fixed (still checking one by one to make sure I'm not missing anything)
npm i && lint && build && test passing from scratch

coveralls · 2018-06-12T18:27:53Z

Coverage decreased (-0.04%) to 94.729% when pulling 6553967 on cancerberoSgx:structure into a7c79c1 on dsherret:dev.

dsherret

Hey @cancerberoSgx, looks great overall! Thanks a lot! I made a lot of comments because there's still a bit of work to do here, but hopefully it will help out. I think you're on the right track.

By the way, if at any time you want to merge this I can merge it into a branch and then I'll pick up the work to finish it off.

dsherret · 2018-06-12T23:22:25Z

package-lock.json

@@ -16,7 +16,7 @@
    "@mrmlnc/readdir-enhanced": {
      "version": "2.2.1",
      "resolved": "https://registry.npmjs.org/@mrmlnc/readdir-enhanced/-/readdir-enhanced-2.2.1.tgz",
-      "integrity": "sha512-bPHp6Ji8b41szTOcaP63VlnbbO5Ny6dwAATtY6JTjh5N2OLrb5Qk/Th5cRkRQhkWCt+EJsYrNB0MiL+Gpn6e3g==",
+      "integrity": "sha1-UkryQNGjYFJ7cwR17PoTRKpUDd4=",


Can you revert this file? I think it's using an old version of npm which uses sha1 https://stackoverflow.com/a/47834854/188246

dsherret · 2018-06-12T23:28:22Z

src/compiler/base/BodiedNode.ts

@@ -49,5 +50,16 @@ export function BodiedNode<T extends Constructor<BodiedNodeExtensionType>>(Base:

            return this;
        }
+
+        getStructure() {
+            let bodyText = this.getBody().getText().trim();


We'll have to improve this because right now this includes the indentation. If someone sets the body with this text it will be indented double the amount and might include a leading newline.

Probably instead get the statements within the body and then get the text within the source file from statements[0].getNonWhitespaceStart() to statements[statements.length - 1].getTrailingTriviaEnd(). Once that text is found, we'll have to remove the leading spaces from every line (so remove statements[0].getIndentationText() from every line).

dsherret · 2018-06-12T23:31:39Z

src/compiler/base/BodyableNode.ts

@@ -105,5 +106,28 @@ export function BodyableNode<T extends Constructor<BodyableNodeExtensionType>>(B

            return this;
        }
+
+        getStructure() {
+            // TODO: This complication is because we need to remove body text wrapping braces.


See my comment in BodiedNode. We should be able to use almost the same code here, but handle for when body is undefined.

src/compiler/base/ExportableNode.ts

dsherret · 2018-06-12T23:33:54Z

src/compiler/base/ExportableNode.ts

+        getStructure() {
+            return callBaseGetStructure<ExportableNodeStructure>(Base.prototype, this, {
+                isExported: this.isExported(),
+                isDefaultExport: this.isDefaultExport()


Use this.hasDefaultKeyword().

Reason is because this.isDefaultExport() uses the symbols to figure that out, while this.hasDefaultKeyword() looks at only the AST, which is what we're interested in here.

dsherret · 2018-06-13T00:22:05Z

src/tests/compiler/class/methodDeclarationTests.ts

+            // and also recreate the AST using structure and compare getText() of generated code with original node's
+            const aux = sourceFile.addStatements("class __AuxClass{}");
+            const decl = aux[0];
+            if (!TypeGuards.isClassDeclaration(decl)) {


Don't worry about adding this. It's obvious from above that it's a class so just assert it.

dsherret · 2018-06-13T00:22:31Z

src/tests/compiler/class/methodDeclarationTests.ts

@@ -202,4 +221,36 @@ describe(nameof(MethodDeclaration), () => {
            });
        });
    });
+
+    describe(nameof<MethodDeclaration>(d => d.getStructure), () => {
+        function doTest(code: string, expectedStructure: MethodDeclarationStructure) {


Try using MakeRequired<MethodDeclarationStructure>

src/tests/compiler/function/functionDeclarationTests.ts

dsherret · 2018-06-13T00:25:08Z

src/tests/compiler/function/parameterDeclarationTests.ts

@@ -129,4 +130,25 @@ describe(nameof(ParameterDeclaration), () => {
            doTest("function identifier(param, param2, param3) {}", "param2", "function identifier(param, param3) {}");
        });
    });
+
+    describe(nameof<ParameterDeclaration>(d => d.getStructure), () => {
+        function doTest(code: string, expectedStructure: ParameterDeclarationStructure) {


MakeRequired<ParameterDeclarationStructure>

dsherret · 2018-06-13T00:25:32Z

src/tests/compiler/function/parameterDeclarationTests.ts

+        function doTest(code: string, expectedStructure: ParameterDeclarationStructure) {
+            const {descendant} = getInfoFromTextWithDescendant<ParameterDeclaration>(code, SyntaxKind.Parameter);
+            const structure = descendant.getStructure();
+            expect(structure).to.contain(expectedStructure);


We should do an exact comparison in case anything changes. expect(structure).to.deep.equal(expectedStructure).

sure, BTW right now for quickly testing I'm not doing what I've should, this is asserting on the structure object itself instead of re-render a new node asserting against the node.getText() - but at least it gives me some confidence I'm doing things OK and that it works ll the way down to text. Thank you very much for the review - i'm on it! :)

cancerberoSgx · 2018-06-14T15:47:31Z

So, I made all the changes you asked David, with the exception of if (!TypeGuards.isClassDeclaration(decl)), that's my way of type-guard instead of using orThrows and also this one :

Hmmm, could we always use the same base (ConstructorDeclarationBase.prototype), but instead strip the unused properties when this.isOverload() is true? Then we can catch changes to the base that shouldn't be in there in a unit test.

cancerberoSgx · 2018-06-14T15:51:01Z

Regarding:

By the way, if at any time you want to merge this I can merge it into a branch and then I'll pick up the work to finish it off.

I would really like to have this feature in the library since I'm generating a log of code in my projects and basically working with my handcrafted "getStructure" methods. If you think it's more practical that you finish off the work in your own branch then go ahead (I feel you wasted more time in th elast review than if you implemented by yourself.) But to be sincere I would like to keep working on this and give support for getStructure to the rest of the nodes. But primariy, as I said before I would really need this integrated in master ASAP.

Thanks!

dsherret · 2018-06-14T15:55:57Z

@cancerberoSgx I don't view it as wasting time because you taking on this work saves me a lot of time and plus it's another person who learns the inner workings of the library, which is always a plus. I just provided that as an option in case you didn't want to spend the time changing all my comments.

I'm going to be moving over the next week (lots of time spent packing) and attending some family events so I won't have time to work on this for the next while. Please continue working on it 😄

cancerberoSgx · 2018-06-14T16:21:54Z

Nice thanks! BTW I also want to take this opportunity to learn about the inners of the library. I must say it was kind of intuitive once you point me how to workaround that mysterious build error :) Also I must say you are a very careful developer (compared to me) :P What do you think is missing in order to merge this work ? SHould I keep implementing getStructure for the rest of Node kinds ? As I said before I'm kind o fan urgency of having this feature and I woulnd't want to use my own branch... Please advice.

BTW I'm using the library to build TLS plugins for code fixes and refactors common in other typed languages / ides like Java/eclipse. See https://github.com/cancerberoSgx/typescript-plugins-of-mine/tree/master/typescript-plugin-proactive-code-fixes - there are some .gifs at the bottom. I bet you didn't imagine tsa being used like this. It saved me a lot of time although some of the refactors could take more than one second to apply ... Just experimenting and learning - let's see if I reach a production - ready product some day.

THanks!

dsherret · 2018-06-15T14:00:44Z

For it to be merged into master this would have to be complete. So for the definition of complete I would say:

For every currently existing Structure interface there should exist a corresponding .getStructure() method on the node.
For every .getStructure() there should exist a test that checks the return value. <-- we don't need to go over the top and test every combination, but rather just ensure things generally seem to be working (maybe ballpark 1-3 tests per .getStructure() method, but obviously use discretion in the scenario)

If you urgently need this feature, then I recommend publishing your fork to npm and using that package in what you need .getStructure() for.

RE the plugins: very cool! For performance, have you looked into why it's taking so long? Should be pretty quick if you're just looking at the AST and not any symbols or types, but yes, it could be a bit slow otherwise. By the way, see the performance tip I recently added here: https://dsherret.github.io/ts-simple-ast/manipulation/performance -- not sure if it would help though.

cancerberoSgx · 2018-06-15T15:35:40Z

Re plugins performance, I'm creating a new project every time a refactor is applied. Probably this can be improved - but the most slow thing is typeChecker.getTypeAtPosition or type inferring in general - no matter if the project is reused or not. Sincerily didn't put much focus in performance right now but I will, thanks!

src/tests/compiler/interface/interfaceDeclarationTests.ts

dsherret · 2018-09-15T22:04:18Z

This is merged now, but will be released in v15. Thanks for the help @cancerberoSgx! This was so much work.

dsherret · 2018-09-22T22:36:08Z

Hey @cancerberoSgx, this is released now and seems to work well (in 15.0.1). The .fill method is deprecated now though. Use .set instead or the .addX methods.

BREAKING CHANGE: Change to jsx spread attribute structure. Co-authored-by: Sebastián Gurin <sebastigurin@gmail.com> Co-authored-by: David Sherret <dsherret@users.noreply.github.com>

cancerberoSgx added 18 commits May 29, 2018 04:09

getStructure for parameterDeclaration

dc4a3a0

merge master

d4ab6dd

getStructure working

63b205c

getStructure simple test

a90db28

lint

eccab39

FunctionDeclaration getStructure basic

7eeb335

function declaration working

e77b17a

ClassDeclaration getStructure - no tests yet

539057c

merge master

16cb029

lint

757694a

ParameterDeclaration getStructure test

a45ba3d

remove joinStructures

9b05c88

methodDeclaration getStructure test

cb43b7a

getStructure - overrides impl

b29233f

getStructure -abstract methods / prop accessors working

31daea3

class decl getStructure

48abb22

arrowfunction getStructure

641ee6a

lint

dd0df60

getStructure lint

dd0a249

dsherret requested changes Jun 13, 2018

View reviewed changes

cancerberoSgx added 2 commits June 13, 2018 02:55

getStructure bodied and bodyable fix

404bafa

changes suggested by David

4e9d4da

lint-src-and-fix

c519bf1

dsherret force-pushed the structure branch from 45f1ef5 to 00cd097 Compare September 9, 2018 18:28

dsherret added 3 commits September 9, 2018 15:07

Use MakeRequired.

ffe4495

Improve export/import tests.

162276a

Fix FunctionDeclaration.

3ed89cb

dsherret reviewed Sep 9, 2018

View reviewed changes

src/tests/compiler/interface/interfaceDeclarationTests.ts Outdated Show resolved Hide resolved

dsherret added 21 commits September 10, 2018 21:19

Improve parameter and interface tests.

d4f4ff0

Improve variable related tests.

4f68305

Add tests for ConstructSignatureDeclaration.

5d9b8b4

Add tests for IndexSignatureDeclaration.

2fae943

Add tests for MethodSignatureDeclaration.

453d001

Add tests for PropertySignatureDeclaration.

0e35c78

Add tests for NamespaceDeclaration.

c6bfaf8

Add tests for TypeAliasDeclaration.

827d273

Add tests for Decorator.

87b127a

Add tests for JSDoc.

638ac76

Add test for SpreadAssignment.

a3d0adc

Add tests for CallSignatureDeclaration.

5963c04

Add tests for get and set accessor.

2d713bd

Merge branch 'master' into structure.

189ce29

Merge branch 'master' into structure

10cfc32

Add tests for Get and Set fill.

bbebe34

Add tests for PropertyDeclaration.

f7d36ae

Rename files to have correct names.

b70865e

Simplify code verification.

9db0adb

Add getStructure() for some jsx nodes.

de30f12

Update declaration file and remove all "any" return types.

6553967

dsherret merged commit 2d7351c into dsherret:dev Sep 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getStructure #346

getStructure #346

cancerberoSgx commented Jun 12, 2018 •

edited

coveralls commented Jun 12, 2018 •

edited

dsherret left a comment •

edited

dsherret Jun 12, 2018

dsherret Jun 12, 2018

dsherret Jun 12, 2018

dsherret Jun 12, 2018

dsherret Jun 12, 2018

dsherret Jun 13, 2018

dsherret Jun 13, 2018

dsherret Jun 13, 2018

dsherret Jun 13, 2018

cancerberoSgx Jun 13, 2018 •

edited

cancerberoSgx commented Jun 14, 2018

cancerberoSgx commented Jun 14, 2018

dsherret commented Jun 14, 2018

cancerberoSgx commented Jun 14, 2018

dsherret commented Jun 15, 2018

cancerberoSgx commented Jun 15, 2018

dsherret commented Sep 15, 2018

dsherret commented Sep 22, 2018

getStructure #346

getStructure #346

Conversation

cancerberoSgx commented Jun 12, 2018 • edited

coveralls commented Jun 12, 2018 • edited

dsherret left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cancerberoSgx Jun 13, 2018 • edited

Choose a reason for hiding this comment

cancerberoSgx commented Jun 14, 2018

cancerberoSgx commented Jun 14, 2018

dsherret commented Jun 14, 2018

cancerberoSgx commented Jun 14, 2018

dsherret commented Jun 15, 2018

cancerberoSgx commented Jun 15, 2018

dsherret commented Sep 15, 2018

dsherret commented Sep 22, 2018

cancerberoSgx commented Jun 12, 2018 •

edited

coveralls commented Jun 12, 2018 •

edited

dsherret left a comment •

edited

cancerberoSgx Jun 13, 2018 •

edited