-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Added calls to replace_wl_with_plain_text in boxes_to_text and boxes_to_xml for String and Symbol #1136
base: master
Are you sure you want to change the base?
Conversation
…to_xml for String and Symbol
@@ -233,7 +233,8 @@ def __init__( | |||
definitions=None, | |||
output=None, | |||
format="text", | |||
catch_interrupt=True | |||
catch_interrupt=True, | |||
use_unicode=True |
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.
I think this is worng. This is a property of formatting, not evaluation.
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.
@rocky I agree, but I don't know where to place it.
I am concerned about the implications of this PR. Maybe it's better to leave the conversion to the clients, even if we have to traverse the AST twice. |
I found similar problems working with the graph engine. Later I send you a resume of how I think we can face this |
return name | ||
|
||
def boxes_to_xml(self, **options) -> str: | ||
return replace_wl_with_plain_text(str(self.name)) |
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.
Shouldn't this depend on the option passed to the Evaluation object?
@@ -2543,7 +2556,7 @@ def boxes_to_xml(self, show_string_characters=False, **options) -> str: | |||
text = self.value | |||
|
|||
def render(format, string): | |||
encoded_text = encode_mathml(string) | |||
encoded_text = encode_mathml(replace_wl_with_plain_text(string)) |
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.
The same that in Symbol
Well, this took to me more time than I had expected. After working on #1140, and having a closer look at how to implement the proposal in this PR, I think that @GarkGarcia is right: the best choice seems to add a parameter in Evaluation. A second possibility would be to hack the output through the $PrePrint symbol. This is what I did in IWolfram. I used this approach in IWolfram because I needed to make it work also with a WMA kernel, and there I do not have access to the internals. So, if a user tries to overwrite $PrePrint, the interface would get screwed. On the other hand, the Evaluation class, already controls the formatting step. So it would be natural to introduce a property that controls how the final string should be printed. There would be two different ways to control from the front-end the behavior of Evaluation. One is what @GarkGarcia did: just add a string parameter. We could also allow for this parameter a custom call-back function, that takes the standard (WL)UTF8 and process it in the most convenient way for the front-end. |
|
I am working on a better proposition. Once I finish with the checks, I am going to do a PR to this branch. |
96ec58b
to
e8a5440
Compare
b007e8c
to
ef16eb6
Compare
8367d69
to
83bb068
Compare
9570fdd
to
499f1bf
Compare
This is a follow up to https://github.com/orgs/mathics/teams/maintainers/discussions/20/comments/32. This PR makes it so that named characters are only replaced selectively (inside of strings or symbols). This makes calling
replace_wl_with_plain_text
from the clients unnecessary (we only need to setuse_unicode
field ofEvaluation
appropriately).After this is merged, PRs to the clients will follow.