-
Notifications
You must be signed in to change notification settings - Fork 11
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
Remove ASTNode subclassing #533
Conversation
6030f80
to
60401c0
Compare
@schanzer do you have access to code climate settings? I think we should either remove it altogether or at least make it report a "warning" instead of a "failure" in the github check system if at all possible. |
[CodeClimate removed] This is really exciting! Minimizing the surface area of our API like this is going to be a nice improvement. Color me impressed! I'm intrigued by your proposal, to decouple the node methods from the node structure. And I LOVE the idea of using CM6's context-aware rules for it, too. So to try out your CSS analogy, it sounds like we'd get a giant node array and a style object from a language module. The style object would look something like:
...where the last style uses some kind of selector rule to denote that a Is that right? I think that's an incredible idea. Two questions (and feel free to move this discussion to #518 :
|
Potentially yes.
CSS can get so complicated/convoluted that I'm not sure I'd want to sign up for supporting that kind of API, but I think we could support some stricter subset that fits our specific needs more closely. The codemirror 6 API doesn't use literal CSS syntax and doesn't support the full feature range of css selectors. The selector syntax they do support is described here.
Yeah, something like that. The one uncertainty I have about the lezer tree API is how specific child nodes are accessed. Our node spec system provides access via both an ordered list (children()) and via named fields. The named fields make implementing the render/pretty/description functions much easier. The closest thing with lezer trees are node groups, where you can select specific children based only on their node type. Their docs say:
It's unclear whether this was a carefully made design decision that's not going to change or whether it's a feature that just hasn't been implemented yet. |
Oh yeah, I wasn't suggesting we implement the entire day of the selector syntax. Maybe just the most important bits. And I certainly understand the potential benefit of using the same selectors that code Mary users, although that does make us even more implementation dependent. But given the name of the library and our intended uses for it, I don't think that's such a bad thing! As for using Lezer trees natively - given the limitation you describe, it sounds like we'd need to force an invariant like "your style rules must declare fields by name, in the order in which they appear in the source". Then we could just grab the children of each node, name them by matching them to the field array, and pass the resulting object to the renderer. |
Is it alright if I merge this PR or are you still reviewing the code? |
Oh! When you said your work wasn't done I thought you wanted to still push commits. Definitely ready to merge! I'm assuming any necessary changes to the language modules will be tracked in a separate issue? |
I implemented the changes for wescheme-blocks as part of this PR. |
You said it couldn't be done, but here it is!
Here's my before/after summary:
before:
after:
While this PR is good enough to merge, I don't consider my refactoring to be "done", but it's on the path to "done". The next step are:
new ASTNode()
and have the function just return the object.I've also been playing around with codemirror 6 on the size and seeing how they link the parse trees from their lezer system to syntax highlighting. We should do something similar. You basically just have a mapping from ast node type to a highlight type, except the mapping is context aware like css, so identifiers can have different colors depending on the context in which they appear in. We could use the same pattern to have context aware block rendering.