adamretter commented on issue #46: URL: https://github.com/apache/commons-codec/pull/46#issuecomment-617230193
@aherbert After your initial PR feedback which seemed positive, your second comments are quite disheartening. I tried to reuse what was already present in Commons Codec, i.e. the `Hex` class, which I though seemed out of place, but I thought you must have a good reason for having it. I could of course refactor this code to not use that, but... I also have some confusion/concerns around your other feedback... > This would seamlessly support decoding of lower and upper case hex characters using a decode table. Any character not in the decode table is ignored as whitepsace/padding. I don't understand why anyone would want that? Whilst with Base64 it makes sense because there are standards we can point to, which say that is what should happen, there are no such standards for Hex. If I started encoding or decoding data to hexadecimal and I encountered not alphabet chars (e.g. whitespace) my expectation is that that is an error, because they are not in the hexadecimal alphabet and there is no standard I am adhering to that says to ignore them. This seems very unintuitive to me and the source of possible unexpected data bugs for users. > Your current class cannot decode whitespace padded Hex or encode with a line wrapping separator. I would expect to be able to encode to Hex wrapped at 72 characters: I am wondering where that expectation of 72 characters comes from? Again I don't know of any standard for hexadecimal (unlike Base64) which stipulates that. Certainly as a naive commons-codec user I don't have such an expectation. Perhaps there are some specs for Base16 that I am aware of that you could point me at? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
