Fix up encoding and avoid encode-decode waste. Resolves #82 #86
Conversation
| element.binary_representation = dict.pop("text") | ||
| element.text_representation = str(element.binary_representation) | ||
| element.binary_representation = binary | ||
| element.text_representation = text |
There was a problem hiding this comment.
Hmm. It actually looks like the type of element.binary_representation is wrong. It is Optional[str], but I think it should be Optional[bytes]. Document has it right, so I'm guessing this is just an oversight. @bohou-aryn just confirming there's not a reason you know of? I noticed b/c this is failing type checking since you have correctly assigned bytes.
There was a problem hiding this comment.
(to be clear, the issue I'm referring to is not in the code here, but in the definition of the Element class).
There was a problem hiding this comment.
Makes total sense. That's what I get for only running 2 of the 3 linters locally. We should get Poetry to run all the linters in one command.
There was a problem hiding this comment.
poetry run pre-commit run --all-files should run all the linters, though there are some issues running mypy in pre-commit that I haven't been able to figure out (#83). In the meantime poetry run mypy --install-types . should run mypy in the most reliable way.
There was a problem hiding this comment.
I don't know about that type change, mostly the reason should just be a typo when restructuring. Didn't we decide to only keep the bytes representation? this is postponed to future fix?
No description provided.