Skip to content

Comments

Fix hardcoded zeros for encode(0, ...) and padding#27

Open
sd-yip wants to merge 2 commits intosuminb:developfrom
sd-yip:fix-hardcoded-zeros
Open

Fix hardcoded zeros for encode(0, ...) and padding#27
sd-yip wants to merge 2 commits intosuminb:developfrom
sd-yip:fix-hardcoded-zeros

Conversation

@sd-yip
Copy link

@sd-yip sd-yip commented Feb 2, 2024

Currently, decode(encode(0, charset=MY_CHARSET), charset=MY_CHARSET) may not yield 0 due to hardcoding. This proposed change eliminates hardcoding instances and resolves the issue.

@google-cla
Copy link

google-cla bot commented Feb 2, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sd-yip
Copy link
Author

sd-yip commented Feb 6, 2024

@suminb Would you mind taking a look?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect roundtrips for encode(0, charset=...) and leading-zero byte padding by removing hardcoded "0" assumptions and consistently using charset[0] as the zero digit / marker.

Changes:

  • Update encode(0, charset=...) to return charset[0] instead of the hardcoded "0".
  • Update encodebytes/decodebytes leading-zero padding logic to use charset[0] instead of "0".
  • Add a new test validating encode/decode behavior with a non-default (QWERTY) charset.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
base62.py Removes hardcoded "0" usage by switching to charset[0] for zero and byte-padding markers.
tests/test_basic.py Adds coverage for encode(0) / decode() with a custom charset to prevent regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 50 to 51
# Encode the leading zeros as "0" followed by a character indicating the count.
# This pattern may occur several times if there are many leading zeros.
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment describing leading-zero padding is now inaccurate: the marker is no longer the literal string "0", but charset[0]. Please update the comment so it matches the new behavior (and avoids confusing readers when using non-default charsets).

Suggested change
# Encode the leading zeros as "0" followed by a character indicating the count.
# This pattern may occur several times if there are many leading zeros.
# Encode the leading zeros using charset[0] as a marker, followed by a character
# indicating the count. This pattern may occur several times if there are many
# leading zeros.

Copilot uses AI. Check for mistakes.
Comment on lines +44 to +50
def test_basic_qwerty():
kwargs = {"charset": CHARSET_QWERTY}

assert base62.encode(0, **kwargs) == "1"
assert base62.decode("1", **kwargs) == 0
assert base62.decode("1111", **kwargs) == 0
assert base62.decode("111112", **kwargs) == 1
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds coverage for encode(0) / decode() with a non-default charset, but the PR also changes the non-default-charset behavior of encodebytes() / decodebytes() leading-zero padding. Please add assertions that encodebytes/decodebytes roundtrip (including leading null bytes) with CHARSET_QWERTY (and/or CHARSET_INVERTED) so the padding changes are exercised by tests.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant