Closed andrewemeryanz closed 4 years ago
Couple of things:
I was working on a branch on my fork with the intention of having the parser generate the AST directly (removing the TreeElement type), hard to see if that is what you're aiming for or some other end point?
My opinion is that TreeElement should not be an exported type, the AST node types and the Scanner interface should be the exported API for the parser, this means merging the parser and ast packages (which involves fiddly refactoring which is easy to cause conflicts, so coordination).
Probably worth questioning if the Unparse
functionality is actually required, I think if that were removed moving the parsers to return the AST nodes directly could be acheiavble.
- Having on a single letter difference between two exported packages is slightly unsafe (parse and parser), parse should be renamed to reflect that it is just an interface package or something
Do you have a suggestion?
- I'm curious as to the reason behind this refactoring? What is the end-goal here?
The eventual goal is to have a Term
reference the ast.Node
from which it was parsed. Currently, the ast
package depends on the parser
package. These changes invert this relationship with minimal disruption.
I was working on a branch on my fork with the intention of having the parser generate the AST directly (removing the TreeElement type), hard to see if that is what you're aiming for or some other end point? My opinion is that TreeElement should not be an exported type, the AST node types and the Scanner interface should be the exported API for the parser, this means merging the parser and ast packages (which involves fiddly refactoring which is easy to cause conflicts, so coordination).
No comment on TreeElement
other that it doesn't appear to display clear functionality at present. It was only touched because Scanner
is a TreeElement
.
The eventual goal is to have a
Term
reference theast.Node
from which it was parsed.
why? (maybe we should discuss this in slack)
The purpose is to aid debugging. There’s currently no way to trace a term back to the source if there’s an error in the grammar. The ast.Node
gives us access to the Scanner
file position.
Closing as unnecessary. Prefer a larger refactor of ast
and parser
packages.
Modifies the package dependency order so that
ast
no longer depends onparser
. This is achieved by:parse
package that housesScanner
andTreeElement
.parser
package now dependent onast
.The following public components are affected by a package change: