bpo-30103: Allow Uuencode in Python using backtick as zero instead of space#1326
bpo-30103: Allow Uuencode in Python using backtick as zero instead of space#1326zhangyangyu merged 10 commits intopython:masterfrom
Conversation
|
@zhangyangyu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @doerwalter, @serhiy-storchaka and @warsaw to be potential reviewers. |
|
|
||
| The :func:`~binascii.b2a_uu` function now accepts an optional *backtick* | ||
| keyword argument. When it's true, zeros are represented by backticks | ||
| instead of spaces. (Contributed by Xiang Zhang in :issue:`30103`.) |
There was a problem hiding this comment.
Double space after a period.
| b'$`$-A=```\n') | ||
| self.assertEqual(binascii.a2b_uu(b'$`$-A=```\n'), | ||
| binascii.a2b_uu(b'$ $-A= \n')) | ||
| self.assertRaises(TypeError, binascii.b2a_uu, b'', b'`\n', True) |
There was a problem hiding this comment.
What does this test mean?
There was a problem hiding this comment.
backtick is expected to be keyword-only. But sorry I make some mistake here.
| def encodedtextwrapped(mode, filename, backtick=False): | ||
| if backtick: | ||
| res = (bytes("begin %03o %s\n" % (mode, filename), "ascii") + | ||
| encodedtext.replace(b' ', b'`') + b"\n`\nend\n") |
There was a problem hiding this comment.
Seems the only space in encodedtext is the padding space. It would be worth to change examples so that they include inner spaces.
There was a problem hiding this comment.
Good catch. I found it when write the test too but didn't change it.
|
|
||
| /* Store the length */ | ||
| *ascii_data++ = ' ' + (bin_len & 077); | ||
| if (backtick) |
There was a problem hiding this comment.
May be
if (backtick && !bin_len)
*ascii_data++ = '`';
else
*ascii_data++ = ' ' + (bin_len & 077);?
There was a problem hiding this comment.
It's good. The code seems more clear telling the logic that only zero needs to be specially treated.
| encodedtext = b"""\ | ||
| M5&AE('-M;V]T:\"US8V%L960@<'ET:&]N(&-R97!T(&]V97(@=&AE('-L965P | ||
| (:6YG(&1O9PH """ | ||
| M5&AE(\'-Y;6)O;\',@;VX@=&]P(&]F(\'EO=7(@:V5Y8F]A<F0@87)E("% (R0E |
There was a problem hiding this comment.
I don’t think you need to escape the quotes (\')
| binascii.b2a_uu | ||
|
|
||
| data: Py_buffer | ||
| / |
There was a problem hiding this comment.
Was this intended? If so, it needs documenting and testing, but it would be good to avoid if possible. Can’t you put a slash and asterisk after each other to have positional-only and then keyword-only parameters?
There was a problem hiding this comment.
AC currently seems doesn't support combining positional-only parameters with keyword-only parameters. So it's not intended but a side effect.
There was a problem hiding this comment.
It does, I just checked this.
There was a problem hiding this comment.
While I support keeping data positional-only, but in bpo-25357 b2a_base64 was changed and as a side effect data is able to accept a keyword argument. The consistency that functions in module binascii all support a positional-only data was broken. What's your mind @serhiy-storchaka and @vadmium ? We keep data positional-only or just make it like b2a_base64 or make b2a_base64 data back?
There was a problem hiding this comment.
It looks to me that change in bpo-25357 was not intentional. Perhaps we can revert it. Open a new issue for this.
| UUStdIOTest, | ||
| UUFileTest, | ||
| ) | ||
| support.run_unittest(UUTest, UUStdIOTest, UUFileTest) |
There was a problem hiding this comment.
This seems like an insignificant change. Wouldn’t it be simple to use “unittest.main” instead?
| Convert binary data to a line of ASCII characters, the return value is the | ||
| converted line, including a newline char. The length of *data* should be at most | ||
| 45. | ||
| 45. If *backtick* is true, zeros are represented by backticks instead of spaces. |
There was a problem hiding this comment.
Show the backtick character explicitly, as
``"`"``
And in all other cases where it is mentioned.
|
|
||
|
|
||
| .. function:: b2a_uu(data) | ||
| .. function:: b2a_uu(data, \*, backtick=False) |
There was a problem hiding this comment.
AFAIK the backslash is not needed here.
| if (backtick && !bin_len) | ||
| *ascii_data++ = '`'; | ||
| else | ||
| *ascii_data++ = ' ' + (bin_len & 077); |
There was a problem hiding this comment.
Actually & 077 is not needed. bin_len is not larger than 45.
|
Thanks @serhiy-storchaka and @vadmium ! |
No description provided.