Skip to content

feat: add encoder_by_name factory function - #54

Open
sumanjeet0012 wants to merge 1 commit into
multiformats:masterfrom
sumanjeet0012:fix-issue-39
Open

feat: add encoder_by_name factory function#54
sumanjeet0012 wants to merge 1 commit into
multiformats:masterfrom
sumanjeet0012:fix-issue-39

Conversation

@sumanjeet0012

Copy link
Copy Markdown
Contributor

Fixes #39

Description

This pull request adds an encoder_by_name() factory function, which enables creating an Encoder from a single-character prefix as well as the standard name, bringing the python implementation into alignment with the go-multibase reference implementation.

Changes

  • Created encoder_by_name() in multibase/multibase.py which inspects the given string and resolves it if it matches either a name or a prefix.
  • Exported encoder_by_name from __init__.py.
  • Added tests for encoder_by_name demonstrating successful construction from prefix, name, and base256emoji, as well as proper exception handling for unsupported/invalid strings.

@acul71 acul71 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary (maintainer)

Thanks for the clean encoder_by_name() addition — it matches issue #39 and go-multibase’s name/prefix factory pattern, keeps Encoder.__init__ backward compatible, and local/CI checks look good.

Blocker (must fix)

Missing newsfragment. Please add:

  • newsfragments/39.feature.rst

with a short user-facing ReST note (trailing newline required), e.g. that encoder_by_name() creates an Encoder from an encoding name or multibase prefix character.

Also please address

  1. Rebase onto origin/master — branch is 6 commits behind (dry-run merge currently clean, but please sync).
  2. Input hardeningencoder_by_name(b"f") currently yields encoding == b"f"; non-str values like None raise AttributeError. Prefer requiring str or normalizing to the codec name.
  3. Docs — README still only shows Encoder('base64'); add a brief encoder_by_name example.
  4. Tests (nice-to-have) — assert .encode() via the factory, not only .encoding.

Notes

  • No merge conflicts vs current origin/master.
  • Informational: overlaps with PR #53 on multibase.py / tests.
  • Full review: local downloads/AI-PR-REVIEWS/54/AI-PR-REVIEW-54-0.md.

Requesting changes until the newsfragment (and preferably rebase) is done.

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.

Encoder class only accepts encoding names, not prefix characters

2 participants