-
Notifications
You must be signed in to change notification settings - Fork 143
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
Implement extends #536
base: master
Are you sure you want to change the base?
Implement extends #536
Conversation
5d51e98
to
8a21213
Compare
The new syntax files cause new test to fail (and C# to pass). This should be looked over more carefully and fixed in the future.
This adds support for the `extends` keyword. The implementation is very hacky right now: - We now require an optional `base_syntax` when parsing a syntax, if the `base_syntax` is not present and we find an `extends` keyword at the top level, we error out reporting which syntax file we expected. - SyntaxSet collects this error and resolves the syntaxes in a loop until no more syntaxes can be resolved. - We don't handle multiple-inheritance (`extends` might be a list). - We don't re-evaluate the `contexts` as defined in the spec (you can override variables and have that affect the context). - We only handle `extends` for syntaxes added through `SyntaxSet::load_from_folder`.
8a21213
to
a712440
Compare
I forgot to say this at the time, but thanks very much for your hard work on this! It is very much appreciated! 🚀
The match patterns are currently evaluated when the syntax definition is parsed. In |
I notice there is an idea to delay expansion of variables - #541 (comment) - perhaps we should also consider that as a nicer solution to the problem of allowing extended syntaxes to use the overridden variables? |
This adds support for the
extends
keyword which lets the tests pass on Build 4075 as requested in #323. The implementation is very hacky right now:base_syntax
when parsing a syntax, if thebase_syntax
is not present and we find anextends
keyword at the top level, we error out reporting which syntax file we expected.SyntaxSetBuilder
handles this error and resolves the syntaxes in a loop until no more syntaxes can be resolved.extends
might be a list).contexts
as defined in the spec (you can override variables and have that affect the context).extends
for syntaxes added throughSyntaxSetBuilder::load_from_folder
.I am opening this PR on top of #535 to get the fixes to the tests.
I don't know if this is a correct implementation of the
extends
keyword, specifically, I don't know when contexts are evaluated; the contexts are properly merged, but the variables can be overwritten and thus the context needs to be re-evaluated, I don't know if I need to do something about it, or id the context evaluation is done after theSyntaxSet
is built.I am not happy with the implementation either, but I don't know how to improve it further. I will happily improve it if you have any ideas.
Ideally,
SyntaxSetBuilder::build
would return an error that indicates the syntax was unable to extend another syntax.