-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix and test expressions of the form BoxExpression[[...]] #561
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
@@ -305,6 +305,13 @@ def apply_list(self, boxes, evaluation): | |||
result = RowBox(*items) | |||
return result | |||
|
|||
@property | |||
def elements(self): |
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.
It feels like something in the code or the API is wrong here.
I get nervous about "properties" doing possibly unbounded calculations such as self.to_expressions()
.
If I understand correctly, the caller somehow knows this is a compound Expression/Element and assumes that therefore it has an .element
attribute which is of type Expression
?
It feels like what should be happening that the caller should be asking instead for a conversion, that is, should be calling to_expression().elements
I need to think about this more.
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.
It feels like something in the code or the API is wrong here.
Probably it is, but I tried to build over the existing API. At least, this is "localized" in this class.
I get nervous about "properties" doing possibly unbounded calculations such as
self.to_expressions()
.If I understand correctly, the caller somehow knows this is a compound Expression/Element and assumes that therefore it has an
.element
attribute which is of typeExpression
?
The problem is that Part
assumes that an Expression has a property elements
. This just provides the property by creating an Expression. A different possibility would be to implement a different rule for working with RowBoxes. I thought this change was simpler, but maybe there is a better approach.
It feels like what should be happening that the caller should be asking instead for a conversion, that is, should be calling
to_expression().elements
I need to think about this more.
Sure.
The what is most appreciated. However the how may need some more thought or discussion. |
@mmatera what is the status of this PR? |
@TiagoCavalcante, by now, it is a reminder to something we need to fix. |
Another small fix to something that I found working on MakeBoxes. When a
BoxExpression
is passed as the first element toPart
, since it does not have in general an attribute_elements
, the evaluation ends with an unhandled exception. In particular, this happens toRowBox
. This PR fixes it by providing an implementation for that method.