-
Notifications
You must be signed in to change notification settings - Fork 214
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
Initial v2 refactor #131
Draft
xogeny
wants to merge
4
commits into
jsonata-js:ts_refactor
Choose a base branch
from
mtiller:initial-v2-refactor
base: ts_refactor
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Initial v2 refactor #131
Commits on Jan 22, 2018
-
Lots of changes here. First and foremost, this organizes the code into es6 modules. I tried to hit the big functional bigs: tokenizer, parser, evaluator, functions and separate them out. The parser was particularly involved so I actually further decomposed that so that files were at least a reasonable size. I don't have particularly strong feelings about the taxonomy withing the parser package, just trying to get things to a manageable state. There are several other big changes worth noting here in the tooling. First, I migrated the code to TypeScript. I think TypeScript is a particularly good fit for AST related work because of the tagged union support. But it also really helps with refactoring so I think it will be pretty useful moving forward. It all compiles out to es6 and es5 and can be bundled with rollup. So it doesn't impact *deployment* at all, just development. I moved the tests to `jest`. I find Jest is a little easier to configure out of the box and things like coverage are well supported. I used `yarn` to create a `yarn.lock` file. We could also use `npm` `v5`, but since `jsonata` currently supports Node `v4`, and `npm` `v5` only ships with Node v7+. Bottom line is that `yarn` (or `npm5`) give much better reproducibility on installs. Even with all these changes, the test suite still passes 100% of tests with 100% coverage!
Configuration menu - View commit details
-
Copy full SHA for b8ffa23 - Browse repository at this point
Copy the full SHA b8ffa23View commit details -
Configuration menu - View commit details
-
Copy full SHA for f8ee6ec - Browse repository at this point
Copy the full SHA f8ee6ecView commit details
Commits on Jan 23, 2018
-
Configuration menu - View commit details
-
Copy full SHA for 33d5430 - Browse repository at this point
Copy the full SHA 33d5430View commit details
Commits on Jan 25, 2018
-
More work, mainly on typing related aspects
One big priority was removing optional fields from AST nodes and tagging AST nodes with `global` properties (like `keepArray`, see below). I haven't changed the AST definitions at all. The `type` and `value` fields should have the same values they always had. However, in the types I'm using tagged unions for all the AST nodes. Generally, the `type` field is used to narrow the type. But in some cases, I further refine type by `value` (which you can really think of as a subtype for the AST nodes). This basically allows me to narrow the type further in the code and do more static checking during compilation to make sure that the proper fields are being read and/or written. One "breaking change" is with the parser recovery tests. These aren't really breaking in the sense that they would impact most users. They have to do with including some additional lexical information. This information was, generally speaking, already there. But there were cases where it wasn't included for some AST nodes. So the change is really just including it consistently. Another noticeable change is with error handling. Previously, the syntax errors were annotated on the root node of the AST. I've changed that so they don't impact the AST. I've also changed the error semantics very slightly. Now, if there are no errors, the `errors` field in an expression will return an empty array (previously, it returned `undefined`). One completely internal change was the elimination of the `keepArray` attribute on AST nodes. This was added whenever the `[]` was found in an expression. I've changed the handling so that instead of having the parser walk through the AST and find the head of the path that contains the `[]` operator and then set the `keepArray` attribute on it, I'm simply adding a special "decorator" AST node that is optimized away eventually. This keeps the parser focused just on building the initial AST and leaves the semantics of the `SingletonArrayDecorator` (the node that gets inserted as a result of an `[]` in a path) to be handled completely during `ast_optimize`. It is important to note that this special AST node is always optimized away. In `ast_optimize` I generate a new AST node as a result of optimization in each `case` of the `switch` statement. This is mainly to keep the typing clean (so we know precisely what we are working with at any given time).
Configuration menu - View commit details
-
Copy full SHA for f5420d9 - Browse repository at this point
Copy the full SHA f5420d9View commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.