Skip to content

refactor: use direct Markdown subclass #6

Description

@redtux

Restore direct Markdown subclass; fix cold-import at the test boundary

Background

Plan 10 (collapse-impl-module) replaced the two-file __new__-redirect
architecture with a single-file abc.ABC + register() virtual-subclass
pattern, to fix a real circular import while also fixing the isinstance
breakage the old two-file split had introduced.

The traceback that motivated Plan 10's approach:

tests/test_spdx_markdown.py:12
  → src/spdx_markdown.py:20  from commitizen.changelog_formats.markdown import Markdown
  → .venv/.../changelog_formats/__init__.py:59  ep.name: ep.load()
  → importlib/metadata/__init__.py:181  functools.reduce(getattr, attrs, module)
  → AttributeError: partially initialized module has no attribute 'SPDXMarkdown'

This showed the cycle is real — but reproduction since then revealed
its scope is narrower than initially assumed.

Scope of the circular import

Two scenarios were tested with the direct-subclass version (no wrapper):

Approach A — direct subclass Approach B — ABC wrapper
cz bump --dry-run --changelog — ✅ works ✅ works
Cold python -c "from ...spdx_markdown import SPDXMarkdown" — ❌ AttributeError ✅ works
Same cold import with import commitizen.changelog_formats first — ✅ works ✅ works
pytest collection — ❌ AttributeError (cold path) ✅ works (via conftest noneeded)
isinstance(formatter, SPDXMarkdown) — ✅ true (real inheritance) ✅ true (virtual subclass)

The cycle only manifests when our own plugin module is the first
thing in the process to import commitizen.changelog_formats. cz's
own CLI bootstrap always imports that package before scanning
third-party commitizen.changelog_format entry points, so production
usage was never actually at risk. pytest, run directly against the
test module, is the one cold-import path in our own toolchain.

Proposal

  • Replace the abc.ABC / __new__ / register() wrapper in
    spdx_markdown.py with a plain class SPDXMarkdown(Markdown):
    same __init__, get_metadata, and get_latest_full_release
    overrides as the current _SPDXMarkdownImpl, just renamed and made
    the only class in the module. Remove import abc, _resolve(),
    __new__, and SPDXMarkdown.register(...).
  • Fix the cold-import at the one place it actually occurs — test
    collection — by adding a one-line warm-up import to
    tests/conftest.py:
    import commitizen.changelog_formats  # noqa: F401  — warm up before plugin import
  • Mark docs/plans/10-collapse-impl.md as superseded with a
    one-line note linking to Plan 11.

Follow-up tasks from #4 / PR #5

Two issues were spotted in the CI and changelog after PR #5 was merged.
These are unrelated but small enough to include in this change:

  • .github/workflows/publish.yaml — Reorder so GitHub Release
    creation happens before PyPI publish; switch from
    generate_release_notes: true to body_path: release-notes.md;
    quote tags glob pattern [v*] as ["v*"] for YAML compliance;
    fix indentation and remove stale comments.
  • docs/changelog.md — Escape underscore in _impl.py so it
    renders correctly as literal text.

Acceptance criteria

  • spdx_markdown.py contains one class, no wrapper, no register().
  • isinstance(formatter, SPDXMarkdown) is True via real inheritance.
  • tests/conftest.py contains the warm-up import.
  • ruff check src/ — zero warnings, no noqa: E402 needed.
  • uv run pytest -v — all tests pass unchanged.
  • cz bump --dry-run --changelog succeeds.
  • docs/plans/10-collapse-impl.md marked superseded with link to
    Plan 11.
  • publish.yaml and changelog.md fixes applied.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions