kasperkarlsson
commented
6 years ago Hi there! This PR seems to attempt to hard-code all messages sent to always be at exactly 8 bytes long, thus removing support for shorter messages from the tool. It also seems to contain logical bugs.
The parse_messages function
The padding function call in line 54 is only reached if the message length is exactly 0 or strictly greater than 8 (check in line 52). Messages with a length >8 are, however, no longer caught as invalid. The ValueError for "Invalid data length" has been replaced by a method which will accept any length >=8. Basically, (invalid) messages with a length of exactly 0 are padded to 8 bytes by parse_messages, while (valid) messages with length 1 to 8 are not padded and (invalid) messages longer than 8 bytes are simply let through. I fail to understand the reasoning behind this logic, with regards to both intention and implementation.
The send_messages function
Here, all messages are padded to be at least 8 bytes long. This removes the support for sending (valid) messages with length 1-7, without adding any value as far as I can tell.
Arbitrary delimiters
I do not see the purpose of importing the string module, just to allow so many different delimiters in line 43.
>>> print(string.punctuation)
!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~Why should all of these be considered valid delimiters?
Conclusion
Before you start fixing logical bugs - which problem does this PR intend to solve? Why should a user never be allowed to explicitly send messages shorter than 8 bytes? If we cannot find a convincing answer to these questions, this PR will be closed without merge.
Added padding function for when message data is less than 8 bytes. Removed the old delimiter and added support for any (special character) or no delimiter.