pyqrcode / pyqrcodeNG

Python 2 and 3 module to generate QR Codes
BSD 3-Clause "New" or "Revised" License
12 stars 6 forks source link

Better handling of pure binary content #13

Closed albatros69 closed 4 years ago

albatros69 commented 4 years ago

In the original code, it tries to decode the content, based on the guessed encoding, which is bound to fail sometimes if the content is pure binary. If the user has specified the binary mode and the content is already bytes, we should trust him and take the content as is.

heuer commented 4 years ago

Thanks for your contribution Do you mind to add test cases for your code changes?

albatros69 commented 4 years ago

Hi I tried to write a minimal test case, to demonstrate the use case. This is to circumvent the algorithm which tries to detect encoding only when the content is already binary and the mode is forced to binary.

heuer commented 4 years ago

Thanks again. However, I wonder if this is necessary. If you provide an encoding, you'll get the same result:

def test_binary_provide_encoding():
    encoding = 'iso-8859-15'
    data = 'Émetteur'
    qr = pyqrcodeng.create(data, encoding=encoding)
    assert qr.data == data.encode(encoding)
    assert qr.mode == 'binary'
    assert qr.encoding == encoding

Your solution sets the encoding property of the QRCode to None which may be an unexpected value for other users.

Perhaps, the following should work (it does not yet, because PyQRCodeNG decodes the provided data)

def test_binary_provide_encoding2():
    encoding = 'iso-8859-15'
    data = 'Émetteur'.encode(encoding)
    qr = pyqrcodeng.create(data, encoding=encoding)
    assert qr.data == data
    assert qr.mode == 'binary'
    assert qr.encoding == encoding

See https://github.com/pyqrcode/pyqrcodeNG/blob/develop/pyqrcodeng/builder.py#L97

        if isinstance(content, bytes):
            self.data = content.decode(encoding)
        # Give a string an encoding
        elif hasattr(content, 'encode'):
            try:
                self.data = content.encode(self.encoding)
albatros69 commented 4 years ago

Sure, providing an encoding is a possible workaround, provided that the data are encoded data. If this is not the case, pyqrcodeng is currently failing trying to guess the encoding.

heuer commented 4 years ago

Thanks for your comment and the updated test case. Now, I understand the problem much better.

I'll update the lib asap (worst case: Jan 2020)

albatros69 commented 4 years ago

Great! Thanks a lot for the support and the work maintaining the code. Regards,

heuer commented 4 years ago

Merged into the develop branch. Thank you!