Don't throw away visit result in BasePlanProtoVisitor
#118
Conversation
There was a problem hiding this comment.
I argued with myself whether it should be accessible (expecting that folks would need to pull the data out some other way or could decide to enter in a different place). Glad to have an opinion to help push the design one way or the other!
Related piece of information not useful to this review: I believe that this visitor class can be fully automatically constructed (useful to keep in line with the proto definition) but it can also be generated for any given proto as well. I've seen folks asking for generic protobuffer visitors so I know that there is demand. Now if I can only find the time...
|
Apparently I don't have the ability to update the branch and it won't let me approve the PR until the tests pass. Could you rebase the PR for me please? Thanks! |
`visitPlan` is marked `protected`, meaning that non-subclasses of `BasePlanProtoVisitor` (which is probably what'll initiate the `visit` most of the time) cannot have access to the `visit` result. Is there a specific reason that the traversal result has been hidden? If not, i suggest this change. If so, subclasses are obviously free to implement a `visitWithResult` function to do the same thing.
visitPlanis markedprotected, meaning that non-subclasses ofBasePlanProtoVisitor(which is probably what'll initiate thevisitmost of the time) cannot have access to thevisitresult.Is there a specific reason that the traversal result has been hidden? If not, i suggest this change.
If so, subclasses are obviously free to implement a
visitWithResultfunction to do the same thing.