Load encoding files - #1882
Conversation
|
LGTM. I'm kind of glad to see this happening. Algunos comentarios están en español, pero probablemente sea bueno para practicar. |
| # see https://docs.python.org/2/library/codecs.html#standard-encodings | ||
| "ASCII": "ascii", | ||
| "CP949": "cp949", | ||
| "CP950": "cp950", |
| try: | ||
| load_encoding_table(value.value, evaluation) | ||
| evaluation.definitions.set_ownvalue("System`$CharacterEncoding", value) | ||
| except EncodingNameError: | ||
| pass |
There was a problem hiding this comment.
It would be nice to start moving code like this into mathics.eval.atomic.strings
(Whether the parameter checking gets moved there is kind of squishy; I don't really know. It would depend on how it might be used other places.)
There was a problem hiding this comment.
I guess it is fine to do parameter checks inside the eval_ methods, when it helps to decide if the parameters are acceptable for some of the implemented eval_ functions. An eval_ function should always receive suitable parameters. For this particular case, I do not know either: This just sets the encoding, so adding an extra call may be excessive.
There was a problem hiding this comment.
An
eval_function should always receive suitable parameters.
What I've come to see from use is that in some cases it can be rather involved to check validity. This parameter is only valid when the data is of a certain form, which is easier to check at the Python level, as opposed to at the S-Expression level.
Checking for compatibility of matrices in matrix multiplication would be an example of this.
For this particular case, I do not know either: This just sets the encoding, so adding an extra call may be excessive.
You have to consider also whether somewhere else in the code, it will be desirable to do the equivalent of the WL construct as part of its evaluation.
Here, that would be:
$CharacterEncoding := ...While this is probably unlikely, I can imagine it might happen that some other built-in, as a side effect sets the $CharacterEncoding.
But rather than spend mental effort on stuff like this, I think it is better just to follow this pattern and not have to worry about such stuff. It also makes it easier for someone else to follow the code and pattern and do things that way.
| # "noopen" matches WMA. This is nonsensical. | ||
| evaluation.message("Get", "noopen", encoding) | ||
| py_encoding = py_current_encoding | ||
| try: |
There was a problem hiding this comment.
Notice that this code is pretty similar to the code mentioned above that was suggested to be moved into mathics.eval. So maybe there should be a function for the try block.
Not sure how to handle the small differences, though.
There was a problem hiding this comment.
What I tried to do here was avoid the extra call for "native" encodings. But it probably should be written differently, using an exception.
There was a problem hiding this comment.
I am trying to rewrite this code in order to use the stdlib custom encodings that now produce wl_charmap_codec.py. It is included in the following PR.
| return String(boxes.to_text(evaluation=evaluation, encoding="Unicode")) | ||
|
|
||
| if encoding not in ("utf8", "utf-8", "unicode"): | ||
| try: |
There was a problem hiding this comment.
Another try block wrapping load_encoding_table; this should definitely be isolated in a function in eval.
| el interprete real para Klingon.wl / ISO8859-1.m / ISO8859-8.m. | ||
|
|
||
| Correr con: python3 test_wl_charmap_codec.py | ||
| Tests for wl_charmap_codec.py as pure toolkit: does not read .wl files |
There was a problem hiding this comment.
I looked at all of the translations in commit aaf000c. They are pretty good!
Ja, sorry. I just forgot to translate them to English, or just remove them. I was doing experiments with Claude IA to build a part of this code, and as my interactions were in Spanish, it left comments in Spanish. Also it seems it left some "ideosincratic" lines, like add "futures" import :) |
Removed unused future import for annotations.
It's better to translate the comments or leave them in than remove them. Especially if AI is assisting, because it can make mistakes and the comments help verify whether the intent was correct or incorrect. |
Sure! But some of the comments were related to the prompt I used to generate the code. In any case, I supervised most of the process: I just wanted to see how Claude produces code compatible with stdlib codec functions. |
Just a small spelling typo noticed while going over encoding. Co-authored-by: Juan Mauricio Matera <matera@fisica.unlp.edu.ar>
| pass | ||
|
|
||
| return String(boxes.to_text(evaluation=evaluation, encoding="Unicode")) | ||
| # If the encoding does not already exists, try to load it and do it again. |
There was a problem hiding this comment.
I think this way makes more sense...
2 participants
This PR start allowing to load character encodings from the WL encoding files. Also, generate Python encodings that can be used to read/write file operations.