Modify byte decoding #286
Modify byte decoding #286
Conversation
lukaspie
force-pushed
the
strict-string-check
branch
from
f454b85
to
d6eabbb
Compare
dev_tools/utils/nxdl_utils.py
Outdated
| """Decodes a byte array to string if necessary""" | ||
| __error_msg = f"Unsupported type {type(elem)}. Expected bytes, or str." |
There was a problem hiding this comment.
The idea of the original function was that you can feed all types, and it would just return the same value, and only act on byte strings. Now, you have to include type selection everywhere. I don't see the clear advantage. There is anyways no proper type annotation and checking in these parts of the code.
There was a problem hiding this comment.
You are right, here we can just use byte decoding, so I'll remove the error. We wanted to try something similar in pynxtools, but only for string, bytes, and np arrays, so I got a bit confused. I'll change it back here.
There was a problem hiding this comment.
I changed it, so now the only changes are a bit of error checking, docs, and a new test.
There was a problem hiding this comment.
I saw the other PR as well. But there, similarly, a lot of type checking would need to be added everywhere in the code, which I don't clearly see the benefit of.
There was a problem hiding this comment.
I have also changed that one and removing type checking. I initially misinterpreted what you are using this function for, sorry.
Here, we have the same functionality as before, except list of bytes are also decoded. In the other PR in pynxtools, I am also adding support for decoding np array of strings (because NIAC does not want to have numpy in their requirements).
| @@ -194,3 +194,41 @@ def test_namefitting_precedence(better_fit, better_ref, worse_fit, worse_ref): | |||
| assert nexus.get_nx_namefit(better_fit, better_ref) > nexus.get_nx_namefit( | |||
| worse_fit, worse_ref | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
didn't we wanted to put the test to pynxtools?
There was a problem hiding this comment.
The problem is: I cannot just copy all the functionality to pynxtools and just call the function here, then I'll get an import error. But since we don't want numpy here, I ended up splitting the functionality. Here, it checks for bytes and list of bytes and in pynxtools, it calls this function from the nxdl_utils, but on top also performs conversion on np arrays.
Therefore, the tests here do the checking for the bytes and list decoding.
Following up on discussions in #283 (comment)