-
Notifications
You must be signed in to change notification settings - Fork 6
Semi rewrite #43
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
base: master
Are you sure you want to change the base?
Semi rewrite #43
Conversation
Does this superseed #41 btw? If so we can close that |
Not necessarly, this is a different approach to the rewrite. I still think there could be value in a streaming parser, especially for big configs. I am gonna rebase that one and possibly work on it in the future once this is merged |
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.
Overall impl looks great, very clean code. A few areas that do need attention, but they are all minor stuff really
assets/inputs/integer.corn
Outdated
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.
We'll need to update the spec to include the additional features before merge. Obvs gonna rework that at some point, but for now changes can just go on a v0.11
branch.
(Same is true of the interpolation/unicode syntax changes)
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.
Should we have a dedicated spec repository or include the spec in here? Because the current one lives in the website afaik.
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 reckon its own repo. While I want to keep this the reference implementation I think it makes sense to have the spec elsewhere, mainly so it can be managed separately (ie issues/PRs are directly against spec only)
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.
Empty repo, details TBD https://github.com/corn-config/specification
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.
Code all LGTM as it stands. Will await the final changes and then fully re-review.
This PR is a major rewrite of the parser that comes with many improvements and changes. At its core, the main change is the underlying technology.
Instead of using a Pest grammar which is hard to maintain and read, the new parser separates the steps into a proper lexer using Logos and a simple but very efficient parser using LALRPOP. This gets turned into an AST which also allows for more efficient and streamlined implementation of the deserializer.
The parser is largely backwards compatible except where the old parser was wrong. The new parser should be more correct, and some tests have been updated to reflect that. The value enum has also been updated to be more ergonomic and easier to use.
There are a few breaking changes that improve the syntax and were agreed upon beforehand:
truefalse
won't work anymore"$var"
→"${var}"
"\u{FFFD}"
Enhanced Integer Parsing Support:
There might be other changes that are more of a result of the older parser not properly respecting the spec at times.
The changes should address all the concerns highlighted in #42.
There are other changes planned and more extensive error handling which will be implemented in other PRs. The new code should allow much easier extensibility.
Lua is fully working, the CLI has been updated too. WASM support is now fully working and fixed.
Overall, the new code is more readable, maintainable, correct, more efficient and possibly even faster, although that wasn't the goal of this rewrite.
Resolves #42