Skip to content
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

Restructure Expression objects #1076

Closed
kitchoi opened this issue May 12, 2020 · 4 comments · Fixed by #1093
Closed

Restructure Expression objects #1076

kitchoi opened this issue May 12, 2020 · 4 comments · Fixed by #1093
Labels
priority: high High priority for current milestone / sprint type: refactor
Milestone

Comments

@kitchoi
Copy link
Contributor

kitchoi commented May 12, 2020

This issue proposes restructuring the internal data structures supporting Expression introduced in #1067. As it stands, this should not have any impact on the user-facing API.

An initial attempt has been made in this commit: 5657dd4
For future proofing (as I don't trust GH not to garbage collect this), here is the patch created from this commit:
0001-Restructuring-Expression-not-yet-tested-with-recursi.patch.txt

While this new structure seems to work now, it has not been tested for future proofing with recursion support. Recursion is one that has profound impact on the data structure, and therefore it would be worth testing the proposed structure with recursion, unless it is decided that recursion will never be supported.

For reference, on_trait_change supports "*" for recursion support, namely "root.node*.value" will match root.node.value, root.node.node.value, and so on. It was decided that as a first cut implementation, observe will not support this feature.

@kitchoi kitchoi added this to the 6.1.0 release milestone May 12, 2020
@mdickinson
Copy link
Member

I don't think recursion is relevant here: the expression itself is not recursive, even when the data structure it describes is. An example like root.node*.value might look something like NameExpression("root").then(OneOrMore(NameExpression("node")).then(NameExpression("value")) in the expression language. It's unusual for the data structure representing an expression to be anything other than a tree.

@kitchoi
Copy link
Contributor Author

kitchoi commented May 12, 2020

I might not have used the right words to describe the challenge I faced when I last tackled recursion support here. I just wanted to mention that the feature has been proposed and deferred, and therefore it is worth making sure that the refactoring we attempt is still going to support this when it is needed.

@mdickinson
Copy link
Member

I think we shouldn't be worrying about future recursion support. If we keep the design clean, we'll be able to adapt it to add that support if and when it's needed.

@kitchoi
Copy link
Contributor Author

kitchoi commented May 12, 2020

I am sure it will work, but I'd still like to check if that's okay.

@mdickinson mdickinson added priority: high High priority for current milestone / sprint type: refactor labels May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high High priority for current milestone / sprint type: refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants