refactor(converter): introduce FormatRegistry to centralize format dispatch - #434
Merged
Conversation
…ess code drift Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…erter registry Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…l formats Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…or consistency Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…prove function naming consistency Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…gistry for support checks Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ormatting for consistency Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…spatch and eliminate scattered switch statements Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…r consistency Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…tements, centralize format handling with DefaultRegistry Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ormat retrieval and improve test assertions Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
… format management Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…rForFormat function Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…teToWriter methods for improved output handling Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…to eliminate hardcoded switch statements Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ps and enhance format descriptions Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…riginal registry Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ved flexibility Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ndler registration and retrieval Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…r format resolution Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…on and integration details Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…utput formats and aliases Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…case insensitivity Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…tRegistry Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…and aliases in CoreProcessor Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
8 tasks
dosubot
Bot
added
the
size:XXL
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a centralized FormatRegistry and FormatHandler API; replaces scattered format switches with registry-driven lookups across CLI, generator, and processor layers; adds text/html formats and exported converter helpers; updates tests, config validation, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmd/convert)
participant Registry as FormatRegistry (internal/converter)
participant Generator as HybridGenerator
participant Handler as FormatHandler
participant FS as File/Writer
CLI->>Registry: Get(format)
Registry-->>CLI: handler or ErrUnsupportedFormat
CLI->>Generator: Generate(data, opts, format)
Generator->>Registry: handlerForFormat(format)
Registry-->>Generator: handler
Generator->>Handler: Generate(data, opts)
Handler-->>Generator: output
Generator-->>CLI: output
CLI->>Handler: FileExtension()
Handler-->>CLI: ".ext"
CLI->>FS: write file
FS-->>CLI: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
39 tasks
coderabbitai
Bot
added
documentation
Contributor
Merge Queue Status
This pull request spent 3 minutes 12 seconds in the queue, with no time running CI. Required conditions to merge
ReasonThe pull request #434 has been manually updated HintIf you want to requeue this pull request, you can post a |
…spatch and eliminate scattered switch statements Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/converter/registry_test.go`:
- Around line 429-445: The test compares handler values with assert.Equal in the
loop; change that to assert.Same to verify pointer identity consistency between
the alias-resolved handler and the canonical handler: in the table-driven test
where you call DefaultRegistry.Get(tc.alias) and
DefaultRegistry.Get(tc.canonical) (variables h and canonicalHandler), replace
the final assert.Equal(t, canonicalHandler, h, ...) with assert.Same(t,
canonicalHandler, h, ...) so the test ensures the handlers are the exact same
object rather than just equal.
In `@internal/converter/registry.go`:
- Around line 47-92: In FormatRegistry.Register, add validation to detect (1)
self-aliasing and (2) duplicate aliases returned by handler.Aliases(): while
iterating aliases build a local seen map/set (e.g., seenAlias) and for each
normalized aliasKey panic if aliasKey == key (self-aliasing) and panic if
seenAlias[aliasKey] is already true (duplicate within the same handler); keep
using aliasKeys for the commit phase and do these checks before mutating the
handlers and aliases maps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1db350-062a-4a64-93d0-0d2c69951120
📒 Files selected for processing (10)
cmd/convert.godocs/development/architecture.mddocs/solutions/logic-errors/documentation-code-drift-interface-refactoring.mdinternal/converter/hybrid_generator.gointernal/converter/plaintext.gointernal/converter/plaintext_test.gointernal/converter/registry.gointernal/converter/registry_test.gointernal/processor/README.mdinternal/processor/processor.go
Comment on lines
+429
to
+445
| for _, tc := range tests { | ||
| t.Run(tc.alias, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| canonical, ok := DefaultRegistry.Canonical(tc.alias) | ||
| assert.Equal(t, tc.canonical, canonical) | ||
| assert.True(t, ok) | ||
|
|
||
| h, err := DefaultRegistry.Get(tc.alias) | ||
| require.NoError(t, err) | ||
|
|
||
| canonicalHandler, err := DefaultRegistry.Get(tc.canonical) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, canonicalHandler, h, "alias should resolve to the same handler as canonical") | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use assert.Same for handler pointer identity consistency.
Line 442 uses assert.Equal to compare handlers, but line 245 correctly uses assert.Same for the same semantic check. For handler pointer identity verification, assert.Same is more precise.
♻️ Suggested change for consistency
canonicalHandler, err := DefaultRegistry.Get(tc.canonical)
require.NoError(t, err)
- assert.Equal(t, canonicalHandler, h, "alias should resolve to the same handler as canonical")
+ assert.Same(t, canonicalHandler, h, "alias should resolve to the same handler as canonical")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, tc := range tests { | |
| t.Run(tc.alias, func(t *testing.T) { | |
| t.Parallel() | |
| canonical, ok := DefaultRegistry.Canonical(tc.alias) | |
| assert.Equal(t, tc.canonical, canonical) | |
| assert.True(t, ok) | |
| h, err := DefaultRegistry.Get(tc.alias) | |
| require.NoError(t, err) | |
| canonicalHandler, err := DefaultRegistry.Get(tc.canonical) | |
| require.NoError(t, err) | |
| assert.Equal(t, canonicalHandler, h, "alias should resolve to the same handler as canonical") | |
| }) | |
| } | |
| } | |
| for _, tc := range tests { | |
| t.Run(tc.alias, func(t *testing.T) { | |
| t.Parallel() | |
| canonical, ok := DefaultRegistry.Canonical(tc.alias) | |
| assert.Equal(t, tc.canonical, canonical) | |
| assert.True(t, ok) | |
| h, err := DefaultRegistry.Get(tc.alias) | |
| require.NoError(t, err) | |
| canonicalHandler, err := DefaultRegistry.Get(tc.canonical) | |
| require.NoError(t, err) | |
| assert.Same(t, canonicalHandler, h, "alias should resolve to the same handler as canonical") | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/converter/registry_test.go` around lines 429 - 445, The test
compares handler values with assert.Equal in the loop; change that to
assert.Same to verify pointer identity consistency between the alias-resolved
handler and the canonical handler: in the table-driven test where you call
DefaultRegistry.Get(tc.alias) and DefaultRegistry.Get(tc.canonical) (variables h
and canonicalHandler), replace the final assert.Equal(t, canonicalHandler, h,
...) with assert.Same(t, canonicalHandler, h, ...) so the test ensures the
handlers are the exact same object rather than just equal.
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 1 minute 59 seconds in the queue, with no time running CI. Required conditions to merge
ReasonPull request #434 has been merged manually at 4b0a086 HintYou were too fast! |
39 tasks
coderabbitai
Bot
added
go
unclesp1d3r
deleted the
325-refactorconverter-introduce-formatregistry-to-centralize-format-dispatch-and-eliminate-scattered-switch-statements
branch
March 20, 2026 04:56
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FormatRegistryas the single source of truth for supported output formats, replacing scattered switch statements and constant definitions across 8+ locationsDefaultRegistrycentralizes format names, aliases, file extensions, validation, and generation dispatch — adding a new format requires only registering aFormatHandlerinnewDefaultRegistry()Optionsuniformly, eliminating inconsistent parameter decomposition across handlersprocessor.Transform()viaDefaultRegistry.Canonical()so aliases (txt,htm,md,yml) work consistently across all code pathsStripMarkdownFormattingandRenderMarkdownToHTMLfor cross-package use by the processor.mdfile extension fallback with proper error propagationRegister()for fail-fast validationKey Design Decisions
FormatHandler.Generate()accepts*HybridGenerator(concrete type). This is a dispatch table, not a strategy pattern — acceptable since all handlers are internal. Deferred to interface/closure design if external format registration is needed later.Register()panics: Followsdatabase/sqldriver pattern — init-time panics on duplicate names/aliases/nil handlers.Canonical()passthrough: Returns lowercased input for unknown formats rather than erroring.Get()is the authoritative validation gate.Files Changed
registry.go(new, 268 lines)hybrid_generator.gohandlerForFormat()convert.go,shared_flags.govalidation.goValidFormatsderived from registry withslices.Clone()processor.gooptions.goFormat.Validate()delegates to registryAGENTS.md§5.9b, §12.5registry_test.go,processor_test.go,core_test.goTest Plan
just ci-checkpasses (pre-commit, format, lint 0 issues, all 33 packages green)registry_test.go: 76 test cases covering Get, Canonical, Register panics, ValidFormats, Extensions, handlerForFormat, DefaultRegistry content, handler dispatchopndossier convert --format <TAB>opndossier convert testdata/config.xml -f json -o testCloses #325