[sumanjeet0012]

Description

This pull request removes backwards-compatible APIs and constructor patterns originally introduced to facilitate migration from the legacy pymultihash package. As these compatibility layers have served their purpose and are now obsolete, their complete removal streamlines the codebase and improves long-term maintainability.

Closes #60.

Proposed Changes

• Remove Legacy Constructor: Removes the 2-argument constructor Multihash(func, digest) fallback from the Multihash object.
• Enforce Canonical Initialization: Reverts Multihash to standard namedtuple behavior using strictly (code, name, length, digest).
• Refactor Internal Helpers: Updates digest() and sum_stream() helper functions to explicitly compute and construct the Multihash object using the canonical (code, name, length, digest) signature.
• Test Suite Updates: Removes corresponding test cases that checked the legacy initialization style.

Justification

Removing these legacy wrappers ensures the library only supports the canonical usage patterns, reduces architectural complexity, and brings the implementation closer to native Python standards without carrying unnecessary historical baggage.

[acul71]

Maintainer review (acul71)

Verdict: Request changes

Clean removal of the pymultihash-compatible Multihash.__new__ / Multihash(func, digest) path, with digest() and sum_stream() updated to the canonical 4-field constructor. Local lint/typecheck/test (228 passed)/docs-ci are green; no merge conflicts with master.

This is an intentional breaking change (Closes #60).

Blocker

• Missing newsfragment: please add newsfragments/60.breaking.rst (or 60.removal.rst) describing that callers must use Multihash(code, name, length, digest) and that the pymultihash-style constructor is removed.

Notes

• Overlaps with #59/#62 on multihash/multihash.py — prefer merging those bugfixes first, then rebase this PR
• Confirm no deprecation period is desired (issue #60 describes hard removal)

Please add the newsfragment and we can re-review for approval.

[acul71]

Thanks @sumanjeet0012 for this work!

Brief summary

• Removes legacy pymultihash Multihash(func, digest) compatibility; canonical 4-field construction only (Closes #60).
• Prior blocker fixed: valid newsfragments/60.removal.rst present.
• Lint/typecheck/tests/docs green; no merge conflicts with master (branch is ~23 behind — rebase recommended but non-blocking).
• Verdict: Approve / Ready (Excellent).

Full AI PR review (61-1)

AI Pull Request Review: PR #61

Review Date: 2026-10-25
Review Version: 1 (follow-up to AI-PR-REVIEW-61-0.md)
PR Title: Refactor: Remove legacy pymultihash backward compatibility APIs
Author: sumanjeet0012 (Sumanjeet)
Status: OPEN (maintainer previously requested changes; newsfragment since added)
URL: #61
Closes: #60

---

1. Summary of Changes

This is an intentional breaking refactor that removes the legacy pymultihash-compatible Multihash construction path and restores standard namedtuple initialization.

What changed

• Removes custom Multihash.__new__ that accepted Multihash(func, digest) / Multihash(func=..., digest=...) (and the related 2-arg positional heuristic).
• Enforces canonical construction: Multihash(code, name, length, digest).
• Updates internal helpers digest() and sum_stream() to resolve code/name via FuncReg / constants.CODE_HASHES and call the canonical constructor.
• Drops unused import _is_app_specific_func from multihash/multihash.py.
• Updates class docstring to remove the pymultihash API mention.
• Removes the pymultihash-style assertion from tests/test_funcs.py::test_multihash_construction_styles.
• Adds newsfragments/60.removal.rst (addresses the blocker called out in review 61-0 and by maintainer acul71).

Issues addressed

• Closes #60 — Remove legacy pymultihash backward compatibility APIs (hard removal; no deprecation period described in the issue).

Modules / files affected

Path	Change
multihash/multihash.py	Remove __new__; update digest / sum_stream; docstring
tests/test_funcs.py	Drop legacy constructor test branch
newsfragments/60.removal.rst	New user-facing removal note

Breaking changes / deprecations

• Breaking: Callers using Multihash(func, digest), Multihash(func=..., digest=...), or relying on the 2-arg heuristic will fail (TypeError / unexpected kwargs).
• Not deprecated: Hard removal, matching #60. The Multihash.func property remains (read-only accessor), which is separate from the removed constructor kwarg.
• Canonical 4-field construction and helpers such as digest() / sum() continue to work.

Commits on PR branch (vs merge-base)

1. 0bf1b0c — Refactor: completely remove pymultihash API backwards compatibility
2. 2a67e11 — Refactor: remove leftover pymultihash reference in docstring
3. 4825952 — Add newsfragment for #60

---

2. Branch Sync Status and Merge Conflicts

⚠️ Checked from worktree against origin/master (default branch is master, not main).

Branch Sync Status

• Status: ⚠️ Diverged from origin/master
• Details: branch_sync_status.txt reports 23 3
  • Left (23): commits on origin/master not in HEAD → branch is 23 commits behind
  • Right (3): commits on HEAD not in origin/master → branch is 3 commits ahead
• Compared to review 61-0 (then 9 2), the branch has fallen further behind as master advanced (#59, stream/parity features, dependency bumps, etc.).

Merge Conflict Analysis

• Conflicts Detected: ✅ No conflicts
• Test merge of origin/master into HEAD (merge_conflict_check.log):
  • Auto-merged multihash/multihash.py and tests/test_funcs.py
  • Exit indicated clean stop before commit; merge aborted to restore clean state
• ✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/master.

Note: Clean auto-merge does not mean a rebase is unnecessary for review clarity. Master still contains the legacy __new__ today; this PR’s purpose remains valid. Rebase/merge from master is recommended before final merge for a linear history and to pick up #59 and later multihash.py changes under the PR’s tree, even though conflict risk is currently low.

---

3. Strengths

1. Clear scope and alignment with #60 — Removes exactly the compatibility layer described in the issue (constructor + internal call sites), without unrelated churn.
2. Internal call sites fixed — digest() and sum_stream() no longer depend on the removed constructor, so core helpers keep working after the break.
3. Newsfragment added and valid — 60.removal.rst is user-facing, ends with a newline, uses an allowed towncrier type (removal), and renders correctly in towncrier build --draft.
4. Tests updated; suite green — Local make test: 228 passed; GitHub PR checks (tox core/lint/docs across 3.10–3.14, Windows) all pass.
5. Simpler mental model — Dropping the heuristic __new__ (mixed kwargs, positional reinterpretation, app-specific func handling) makes construction predictable namedtuple behavior.
6. Docstring cleanup — Class docs no longer advertise dual API compatibility.
7. Addresses prior review blocker — Follow-up to 61-0 / maintainer “Request changes” on missing newsfragment.

---

4. Issues Found

Critical

None. The previous CRITICAL blocker (missing newsfragment) is resolved.

Major

None.

Minor

- **File:** tests/test_funcs.py
- **Line(s):** 86–89
- **Issue:** `test_multihash_construction_styles` now only exercises the canonical constructor; the name still implies multiple styles, and there is no explicit regression test that the legacy forms raise.
- **Suggestion:** Rename (e.g. `test_multihash_canonical_construction`) and optionally add `pytest.raises(TypeError)` for `Multihash(Func.sha2_256, b"x"*32)` / `Multihash(func=..., digest=...)`.

- **File:** multihash/multihash.py
- **Line(s):** 572–576 and 893–896 (also `FuncReg.get` already at ~861 in `sum_stream`)
- **Issue:** Near-duplicate code/name resolution when building `Multihash` in `digest()` and `sum_stream()`; `sum_stream` resolves the func twice.
- **Suggestion:** Optional small helper (e.g. `_multihash_from_func_digest(func, digest_bytes)`) to keep one construction path; reuse the already-resolved `func` in `sum_stream`.

- **File:** (branch hygiene)
- **Line(s):** n/a
- **Issue:** Branch is 23 commits behind `origin/master`.
- **Suggestion:** Rebase or merge `origin/master` before merge for a current tip and simpler review of the final tree (no conflicts expected today).

- **File:** newsfragments/60.removal.rst
- **Line(s):** 1
- **Issue:** Type `.removal` is valid and was explicitly allowed by the maintainer; `.breaking` would surface under “Breaking Changes” instead of “Removals.”
- **Suggestion:** Optional preference only — keep `.removal` (current) or rename to `60.breaking.rst` if maintainers want this under Breaking Changes. Not a blocker.

---

5. Security Review

No security concerns identified.

• Removes constructor heuristics; does not weaken digest validation, length checks, or encoding paths.
• No new subprocess, filesystem, crypto, or logging of secrets.
• Canonical construction still requires callers to supply consistent code / name / length / digest (same as other existing construction sites such as decode / from_json).

- **Risk:** None material for this diff
- **Impact:** None
- **Mitigation:** n/a

---

6. Documentation and Examples

• Class docstring updated to drop pymultihash compatibility wording — good.
• Newsfragment documents user impact clearly (constructor removed; canonical args required).
• README / docs/usage.rst: No remaining docs teaching Multihash(func, digest). Historical mention of pymultihash APIs in HISTORY.rst (#19) is appropriate changelog history.
• Optional: A one-line note in usage docs that Multihash is a 4-field namedtuple is nice-to-have but not required given helpers (digest, sum) are the primary API.

---

7. Newsfragment Requirement

✅ Satisfied (blocker from review 61-0 cleared).

Check	Result
Linked issue	✅ Closes #60
File present	✅ newsfragments/60.removal.rst
Naming	✅ <ISSUE>.<TYPE>.rst with allowed type removal
Content	✅ User-facing ReST describing removal of Multihash(func, digest)
Trailing newline	✅ Present (0a at EOF)
towncrier draft	✅ Appears under Removals with #60 link

No additional issues are closed by this PR, so one fragment is correct.

---

8. Tests and Validation

New / modified tests

• Legacy pymultihash-style assertions removed from test_multihash_construction_styles.
• Canonical construction still covered; digest / encode / verify paths still exercised elsewhere.
• Gap (minor): no explicit negative test for removed constructor forms.

Linting (make lint) — exit 0

Log: downloads/AI-PR-REVIEWS/61/lint_output.log

• check yaml — Passed
• check toml — Passed
• fix end of files — Passed
• trim trailing whitespace — Passed
• pyupgrade — Passed
• ruff — Passed
• ruff format — Passed
• mypy (via pre-commit) — Passed

Errors / warnings: none.

Type Checking (make typecheck) — exit 0

Log: downloads/AI-PR-REVIEWS/61/typecheck_output.log

• mypy-local — Passed
  Errors / warnings: none.

Test Execution (make test) — exit 0

Log: downloads/AI-PR-REVIEWS/61/test_output.log

• Collected: 228
• Passed: 228
• Failed / errored / skipped: 0
• Duration: ~0.15s
• Warnings: none reported

Failed tests: none.
Skipped tests: none.

Documentation Build (make docs-ci) — exit 0

Log: downloads/AI-PR-REVIEWS/61/docs_output.log

• sphinx-apidoc + sphinx-build -W succeeded
• Errors / warnings treated as errors: none (build succeeded under -W)

CI (GitHub)

All reported PR checks green (tox core/lint/docs on 3.10–3.14; Windows core/lint).

---

9. Recommendations for Improvement

1. Maintainer re-approval — Prior “Changes requested” was for the missing newsfragment; that is fixed — please re-review/approve.
2. Rebase onto origin/master before merge (23 commits behind; currently conflict-free).
3. Optional test polish — Rename test_multihash_construction_styles; add TypeError regression cases for legacy constructor forms.
4. Optional DRY — Shared helper for (func, digest_bytes) → Multihash used by digest / sum_stream.
5. Optional fragment type — Prefer 60.breaking.rst only if release notes should list this under Breaking Changes; current removal is acceptable per project config and prior maintainer note.

---

10. Questions for the Author

1. Confirm you still want a hard break with no deprecation window (matches #60 and prior maintainer note).
2. After this lands, should any downstream (e.g. py-libp2p) be pinged if it still constructs Multihash(func=..., digest=...)?
3. Prefer newsfragment under Removals (current) or Breaking Changes?

---

11. Overall Assessment

Dimension	Rating
Quality Rating	Excellent
Security Impact	None
Merge Readiness	Ready
Confidence	High

Verdict

APPROVE (code-ready; prior newsfragment blocker resolved).

Previous review 61-0 correctly blocked on a missing newsfragment. This revision adds a valid 60.removal.rst, keeps lint/typecheck/tests/docs green, and implements the #60 removal cleanly. Remaining items (rebase for branch sync, optional negative tests / DRY) are non-blocking.

Comparison to review 61-0

Item	61-0	61-1
Newsfragment	BLOCKER missing	✅ Present & valid
Sync vs master	9 behind / 2 ahead	23 behind / 3 ahead
Merge conflicts	None	None
Local tests	228 passed	228 passed
Merge readiness	Needs fixes	Ready

---

Note on maintainer edits

This PR is from a fork with “Allow edits from maintainers” disabled (maintainerCanModify: false), so we cannot push commits or rebase on this branch for you.

For future PRs, please either:

1. Check “Allow edits and access to secrets by maintainers” when opening the PR, or
2. Open the PR from a branch on multiformats/py-multihash (if you have push access),

so maintainers can help with rebases and small follow-up fixes.