GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-146192: Add base32 support to binascii by kangtastic · Pull Request #146193 · python/cpython Conversation
|
|
You can now update your PR, @kangtastic. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@serhiy-storchaka Already on it 😄 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- Use the new `alphabet` parameter in `binascii` - Remove `binascii.a2b_base32hex()` and `binascii.b2a_base32hex()` - Change value for `.. versionadded::` ReST directive in docs for new `binascii` functions to "next" instead of "3.15"
db96a3f to
bf1308f
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I added some suggestions, but the core LGTM.
Please add assertions for new alphabets in test_constants.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| .. function:: b2a_base32(data, /, *, alphabet=BASE32_ALPHABET) | ||
|
|
||
| Convert binary data to a line(s) of ASCII characters in base32 coding, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is a single line.
I will add wrapcol in a separate issue.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| Convert base32 data back to binary and return the binary data. | ||
|
|
||
| Valid base32 data: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This list is incomplete and redundant. I think it is better to follow the example of ascii85 and base85 (with a reference to the RFC). Mention that the mapping is case-sensitive and no optional mapping of the digit "0" and "1" to letters "O", "I" or "l" is used.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| .. data:: BASE32_ALPHABET | ||
|
|
||
| The base32 alphabet according to :rfc:`4648`. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| The base32 alphabet according to :rfc:`4648`. | |
| The Base 32 alphabet according to :rfc:`4648`. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| .. data:: BASE32HEX_ALPHABET | ||
|
|
||
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. | |
| The "Extended Hex" Base 32 alphabet according to :rfc:`4648`. |
These are the names used in the table 3 and 4 captions in RFC 4648.
Oh, we can even refer directly to the table:
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. | |
| The "Extended Hex" Base 32 alphabet according to :rfc:`4648`, table 4. |
Add this also for Base 64 alphabets if you choose this variant.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if this would come up. RFC 4648 uses all four of the terms "Base 32", "Base32", "base 32", and "base32" to refer to this encoding at various points, but it also states e.g.:
This encoding may be referred to as "base32hex". This encoding should not be regarded as the same as the "base32" encoding and should not be referred to as only "base32".
and e.g.:
One property with this alphabet, which the base64 and base32 alphabets lack...
thus implying that "base32" and "base32hex" are preferred, even if the rest of the document doesn't adhere to the implication.
Anyway, I'll refer to it as "Base 32" in docs for now to fit what's already there, and not reference the table number or touch any Base64 stuff so as to keep the scope of this PR limited.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if len(s) % 8: | ||
| raise binascii.Error('Incorrect padding') |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Should not this be handled in the C code?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): | ||
| s = _bytes_from_decode_data(s) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is only needed if map01 is not None.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Correction: it is also needed if casefold is true, for input like 'ß' or 'ffi'.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if alphabet not in _b32rev: | ||
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I suggest to inline this function. map01 handling is only needed for standard alphabet, and the code for casefold is trivial.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * | ||
| alphabet: Py_buffer(c_default="{NULL, NULL}") = BASE32_ALPHABET | ||
|
|
||
| base32-code line of data. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| base32-code line of data. | |
| Base32-code line of data. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- Update docs to refer to "Base 32" and "Base32" - Update docs to better explain `binascii.a2b_base32()` - Inline helper function in `base64` - Add forgotten tests for presence of alphabet module globals
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Please add also the What's New entry.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| * Contains no excess data after padding (including excess padding, newlines, etc.). | ||
| * Does not start with padding. | ||
| .. note:: | ||
| By default, this function does not map lowercase characters (which are |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Remove "by default". There are no options for non-default behavior.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the user specifying an alternative alphabet be a non-default behavior?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It does not allow to map several characters to the same code.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It would work for folding 100% lowercase input. Maybe "By itself" instead of "By default"? But I don't mind removing it since it's such a rare hypothetical.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): | ||
| s = _bytes_from_decode_data(s) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Correction: it is also needed if casefold is true, for input like 'ß' or 'ffi'.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
- Revise docs - Add whatsnew entry - Minor whitespace change in tests
Referring to a group of 8 bytes as an "octet" may cause confusion, because the term is already commonly used in some languages to refer to a group of 8 bits (i.e. a byte). "Octa" is a suitable preexisting alternative for a group of 64 bits [1] (used by Knuth himself, at that). "Octad" was considered, but it, too, historically refers to a byte. Also rename "quintet" to "quint". "Pentad" was considered, but it historically refers to a group of 5 bits. [1] https://en.wikipedia.org/wiki/Units_of_information
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- Reword NEWS.d entry to "Base32" instead of "base-32". No prior entries have ever mentioned "base-64", etc., but they have mentioned "Base64", etc., so this is more consistent. - Reword whatsnew entry to "Base32" instead of "Base 32". No prior entries have ever mentioned "Base 64", etc., and there is an entry a little further up mentioning "Ascii85, Base85, and Z85", so this is more consistent. - Add a whatsnew entry in Optimizations > base64 & binascii section. - Whitespace change in `binascii.c`.
When decoding invalid length (1, 3 or 6 mod 8) + no padding, mention the invalid length instead of the improper padding in the exception message to match what the base64 decoder does. Additionally, move the logic for setting the exception message (back) outside the "slow path" loop; if we do end up checking canonicity of decoder input, it will feel (subjectively) better to have several checks grouped together after the loop.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
nice work!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@serhiy-storchaka, @gpshead, thanks for the quick review! Doing more of this sort of thing might be fun. Stay safe out there. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…8577 * 'main' of github.com:python/cpython: pythongh-146197: Run -m test.pythoninfo on the Emscripten CI (python#146332) pythongh-146325: Use `test.support.requires_fork` in test_fastpath_cache_cleared_in_forked_child (python#146330) pythongh-146197: Add Emscripten to CI (python#146198) pythongh-143387: Raise an exception instead of returning None when metadata file is missing. (python#146234) pythongh-108907: ctypes: Document _type_ codes (pythonGH-145837) pythongh-146175: Soft-deprecate outdated macros; convert internal usage (pythonGH-146178) pythongh-146056: Rework ref counting in treebuilder_handle_end() (python#146167) Add a warning about untrusted input to `configparser` docs (python#146276) pythongh-145264: Do not ignore excess Base64 data after the first padded quad (pythonGH-145267) pythongh-146308: Fix error handling issues in _remote_debugging module (python#146309) pythongh-146192: Add base32 support to binascii (pythonGH-146193) pythongh-135953: Properly obtain main thread identifier in Gecko Collector (python#146045) pythongh-143414: Implement unique reference tracking for JIT, optimize unpacking of such tuples (pythonGH-144300) pythongh-146261: Fix bug in `_Py_uop_sym_set_func_version` (pythonGH-146291) pythongh-145144: Add more tests for UserList, UserDict, etc (pythonGH-145145) pythongh-143959: Fix test_datetime if _datetime is unavailable (pythonGH-145248) pythongh-146245: Fix reference and buffer leaks via audit hook in socket module (pythonGH-146248) pythongh-140049: Colorize exception notes in `traceback.py` (python#140051) Update docs for pythongh-146056 (pythonGH-146213)
Add base32 encoder and decoder functions implemented in C to the binascii module and use them to greatly improve the performance and reduce the memory usage of the existing base32 codec functions in the base64 module.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
C accelerator for Base32 character encoding
There was an error while loading. Please reload this page.