Skip to content

feat: expose code property on Encoder class - #53

Open
sumanjeet0012 wants to merge 2 commits into
multiformats:masterfrom
sumanjeet0012:fix-issue-47
Open

sumanjeet0012 wants to merge 2 commits into
multiformats:masterfrom
sumanjeet0012:fix-issue-47

Conversation

@sumanjeet0012

Copy link
Copy Markdown
Contributor

Fixes #47

Description

This pull request adds a public .code property to the Encoder class.

Changes

  • Exposes self._codec.code via a read-only property code.
  • Brings py-multibase into alignment with go-multibase, which exposes the encoding code via Encoder.Encoding().
  • Added test validation to ensure encoder.code returns the correct multibase prefix byte.

@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.

Request changes — missing newsfragment (blocker)

Thanks for this clean, focused fix for #47. The Encoder.code property matches the issue proposal and looks correct; CI and local make lint / typecheck / test / docs-ci all pass.

Blocker

Please add a towncrier newsfragment:

  • Path: newsfragments/47.feature.rst
  • Content (example): Added a public ``Encoder.code`` property exposing the multibase prefix byte.
  • Must end with a trailing newline

Newsfragments are mandatory for approval on this repo.

Also recommended (non-blocking)

  • Rebase onto current origin/master (branch is ~6 commits behind; dry-run merge is clean today).
  • Note overlap with open PR #54 on the same files — coordinate merge order.
  • Optional: assert another encoding for .code (e.g. base16 → b"f") and/or mention encoder.code in the README Encoder example.

Happy to re-review once 47.feature.rst is added.

@acul71

acul71 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Hi @sumanjeet0012 — friendly follow-up.

I requested changes on this PR on 2026-09-07 (mainly the missing towncrier newsfragment, plus the other notes in that review). I don’t see any new commits addressing those items yet.

Are you still willing / able to carry this PR forward? If yes, please push the requested updates when you can and I’ll re-review promptly. If your bandwidth has changed, no worries — just say so and we can close it or leave it for someone else to pick up.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>

@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.

Re-review — newsfragment OK; please sync with master

Thanks for adding newsfragments/47.feature.rst — the prior blocker is resolved.

Still needed before approval

Please merge or rebase onto current origin/master. Remote tip 8dc5671 is 10 behind / 2 ahead. I merged master into a local worktree with no conflicts, and all checks passed there, but maintainerCanModify is false so I cannot push the sync for you.

Also helpful: enable “Allow edits from maintainers” on the PR so we can sync next time if needed.

Validation (local worktree after merging master)

  • Encoder.code looks correct; test coverage still present.
  • Local: make lint, typecheck, test (358 passed, 9 skipped), docs-ci — all green.
  • GitHub CI on the current tip: green.

Happy to approve as soon as the branch is synced with master.

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 lacks a code property exposing the prefix byte

2 participants