-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Visualization Tool #71
base: staging
Are you sure you want to change the base?
Conversation
2c17799
to
c8ea564
Compare
71a0789
to
a79ecb7
Compare
@@ -21,7 +21,7 @@ | |||
"tsconfig.json" | |||
], | |||
"scripts": { | |||
"build": "node scripts/copy-folder.js -i src-api/libs -o api/libs && npx tsc -b src-api src-code", | |||
"build": "node scripts/copy-folder.js -i src-api/libs -o api/libs && node scripts/copy-folder.js -i src-api/visualization/public -o api/visualization/public && npx tsc -b src-api src-code", |
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.
Is there a reason for the visualization API being in its own namespace (i.e., src-api/visualization
) instead of being a LARA API (i.e. src-api/lara/visualization
)?
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.
It was just because of what @lm-sousa recommended. In my opinion, it makes sense, because it is a component quite isolated from the rest of the framework, and it is meant to be extended by the compiler and not used directly.
Nonetheless, I am indifferent to this change and it is just a matter of moving the files to a new folder.
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.
Just to confirm, this was a way to circumvent the current limitation of LaraJoinPoint not exposing common attributes such as .code or .children?
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.
Yes, but it also addresses other important points:
- The files inside
public/
are the ones to be sent to the browser via Express.js. This means that the remaining TypeScript files in the project are not accessible to the browser and cannot be used by the webpage's scripts, including theLaraJoinPoint
declaration. Having theToolJoinPoint
allows the web interface to access the AST information, without depending on the remaining classes. - It helps to simplify logic. The webpage only needs a few attributes from the join point classes, and passing only that information heavily decreases the complexity of the conversion to and from JSON, which is needed for the WebSocket communication.
- The existence of the
info
field delegates the choice of the important node information (displayed with the mouse click) to the compiler, helping with the tool generalization.
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.
This file declares functions? Not sure what are the current best practices in Node.JS, I am used to all functions being associated to a class, even if they are static functions, but I have a Java background. Can you confirm this is ok for Node.JS standard coding practices?
Just noticed this code probably is only used by the webpage, and not supposed to be used by LARA scripts, they do not have to strictly follow the same guidelines.
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 wouldn't say that it has to do with Node.js best practices, but rather JavaScript standards. Since it is a scripting language for the web, most commonly the functions are declared globally, instead of as static methods. This is also propagated to Node.js, with exporting the functions directly in the module being a common practice. Also note that classes are fairly recent to JavaScript, only existing since ECMAScript 2015.
Nonetheless, the TypeScript files inside public/
are only scripts for the web interface and are not meant to be accessed from outside LARA, as the professor referred.
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.
Are any of the files/functions in the folder visualization/public
supposed to be used in LARA scripts, or are they exclusively for the visualization webpage? Noticed this because of the import path 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.
Yes, they are not meant to be accessed by the LARA scripts. This folder is copied to the api/
folder to be accessed by the API internally, but it should not be referenced externally.
The only exception is TooljoinPoint.js
, since it needs to be accessed by any implementation of GenericAstConverter
to override getToolAst
.
Description
This PR adds a visualization tool that allows the visual mapping of the source code to the AST. This tool was develop on the context of the summer internship with topic "[CSE01] Visual Mapping of Source Code to Abstract Syntax Tree (AST)", from the INESC TEC Summer Internship 2024 programme.
In specific, this PR adds the generic implementation of the tool, that can be used with any LARA-based compiler that implements the needed classes. After merging this PR, this one can also be merged.
Changes Made
GenericVisualizationTool
), as part of the LARA API, with methods that allows the launching of the tool.GenericAstConverter
) with all the needed operations from the compiler, which every LARA-based compiler should implement to use the tool.How Has This Been Tested
This was mainly tested manually. As the time of writing, I am still doing some final stress testing for possible bugs, so you may see some minor fixes in the future.