-
Notifications
You must be signed in to change notification settings - Fork 55
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
python's repr fixed #828
base: main
Are you sure you want to change the base?
python's repr fixed #828
Conversation
python/tests/test_examples.py
Outdated
@@ -368,6 +368,7 @@ def test_char_vs_string(self): | |||
self.assertEqual(repr(metta.run('!("A")')), '[[("A")]]') | |||
self.assertEqualMettaRunnerResults(metta.run("!(get-type 'A')"), [[S('Char')]]) | |||
self.assertEqualMettaRunnerResults(metta.run('!(get-type "A")'), [[S('String')]]) | |||
self.assertEqual(metta.run('!(repr "A")')[0][0].get_object(), ValueObject("\"A\"")) |
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.
Please move test into test_grounded_type.py
as this is a change in a grounded atom's behaviour.
@@ -279,7 +279,8 @@ def __repr__(self): | |||
# Overwrite Python default representation of a string to use | |||
# double quotes instead of single quotes. | |||
if isinstance(self.content, str): | |||
return f'"{self.content}"' | |||
newstr = self.content.translate(str.maketrans({'"' : r'\"'})) |
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.
Here, all symbols which needs to be escaped should be escaped: for instance "
, '
, \
, <CR>
, <LF>
, <TAB>
other symbols which are not visible. I would suggest finding standard Python function to escape them all.
self.assertEqualMettaRunnerResults(repr(metta.run('!(parse "\\"A\\"")')), | ||
'[[\\"A\\"]]') |
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.
- Test should check which atoms are returned by
parse
not what their string representation is - At the moment test is passing in the following form:
self.assertEqualMettaRunnerResults(metta.run('!(parse "\\"A\\"")'),
[[(S('\\"A\\"'))]])
One can see that now parse
returns symbol instead of string. It is because parse
parses str(atom)
instead of internal string. In this particular case atom is a string "A"
but str()
from this atom gives \"A\"
(this is what is changed by the PR). This sequence is parsed as symbol \"A\"
because escape sequences are processed by MeTTa parser only inside strings and \"A\"
is not a string because it starts from \
symbol. Thus the result of the test is unexpected. To make it proper we need to change parse
implementation to work with original string not with the result of str()
. Test should be passed in its original form:
self.assertEqualMettaRunnerResults(repr(metta.run('!(parse "\\"A\\"")')), | |
'[[\\"A\\"]]') | |
self.assertEqualMettaRunnerResults(metta.run('!(parse "\\"A"\\")'), | |
[[(ValueAtom("A"))]]) |
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.
To properly solve it, I would suggest change parse
implementation to extract Python string from grounded atom on input. Should be as simple as calling .get_object()
on an input atom and checking that this is a really string.
No description provided.