-
Notifications
You must be signed in to change notification settings - Fork 131
Pass comments to AST nodes during parsing #1005
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
Conversation
Teal Playground URL: https://1005--teal-playground-preview.netlify.app |
meta_fields: {string: Type} | ||
meta_field_order: {string} | ||
meta_field_comments: {string: {{Comment}}} |
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.
@upedd It's unfortunate that we have the storage of comments mixed across Node and Type, but I understand why you did it this way.
There is a backstory as to why the parser mixes the creation of Node and Type objects — back in the beginning, it felt like a small optimization. But then when I tried to change everything into Node for a better separation of concerns, it became apparent that the checker now had a dependency on this behavior: this is because the type-checker runs as a single pass of the Node tree, and for some situations it becomes suspiciously convenient that the Type objects are already there during the "first pass" (that is, the processing of Type objects as part of the parser functions like a "hidden zero-th pass").
This is eventually fixable, but not something I want us to do right away because changing that has other consequences to the later checking stage (there are other reasons why I'm considering turning the type-checker into a more (nowadays) common multi-pass process).
So we'll keep it the way you did it for now — this was more for sharing and "documenting" the context behind it.
(cc @Frityet since this is also important for you to know)
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.
For me, this mixing between the Node and Type felt a bit "off". However, it's the best we can do without a major refactor of the parser codebase.
Good to know the context behind it.
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 is in the right direction! I think there's one problem spot I caught on review, and the other big thing is a suggestion for a further improvement.
@upedd Ah, another thing: I think for consistency's sake we should take any comments that you're attaching to Type objects, and copy them in the |
Thank you for your thorough review, @hishamhm. I’ve fixed all the issues you pointed out. Also noticed and fixed some flawed logic in the test code. |
Part of tealdoc project. Passes comments to AST nodes of all declarations, record/interface fields and enum values.