Skip to content

refactor(multimorphic) adopt shared JsonLdProductParser#43

Merged
jkeeley2073 merged 1 commit into
mainfrom
Dev-MultimorphicAdoptsJsonLd
May 3, 2026
Merged

refactor(multimorphic) adopt shared JsonLdProductParser#43
jkeeley2073 merged 1 commit into
mainfrom
Dev-MultimorphicAdoptsJsonLd

Conversation

@jkeeley2073
Copy link
Copy Markdown
Contributor

Summary

Strict-subset follow-up to PR #42. MultimorphicProductExtractor deletes its private 140-line JSON-LD walker and routes through the shared PinballWizard.Infrastructure.Scraping.JsonLd.JsonLdProductParser that JJP and BoF already consume.

Net: −173 lines / +16 lines in MultimorphicProductExtractor.cs. The class now mirrors the post-#42 BofProductExtractor template — same Extract skeleton, same fallback order (product?.Name ?? og:title ?? h1), same EditionInfo shape — modulo the per-storefront prefix / slug rule / DiscoveredOn token.

The class-level <remarks> block that named a "future PR" to extract the shared helper is replaced with a forward reference to JsonLdProductParser. BofProductExtractor's docstring also drops the parenthetical "(when PR #39 lands)" qualifier now that Multimorphic actually consumes the parser.

Why it matters: validates the shared parser against a third real consumer in production code. The test suite from PR #42 already pinned every shape (including the simultaneous-flat-and-nested case unique to Multimorphic), but a third consumer is the signal that the abstraction generalises cleanly.

Test Plan

  • dotnet build → 0 warnings, 0 errors
  • dotnet test → 430 / 430 passing
  • All 27 existing Multimorphic tests pass without modification, including:
    • Extract_RealMultimorphicShape_BuildsRecord (end-to-end live-shape capture)
    • Extract_NestedPriceSpecOnly_StillReadsPrice
    • Extract_GraphWrappedJsonLd_AlsoWorks
    • Extract_MalformedJsonLd_FallsThroughCleanly
  • Behavior equivalence verified by inspection: deleted private parser was a near-identical copy of the now-shared JsonLdProductParser; pre/post comparison against git show HEAD:... confirms GameRecord construction at lines 74-88 is byte-identical.

Out of Scope

  • JJP cleanup follow-up. Local review surfaced two pre-existing JJP drift items (not introduced by this PR): JjpProductExtractor.ExtractSlug is missing the ArgumentNullException.ThrowIfNull(productUrl) guard that BoF and Multimorphic both have, and JjpProductExtractor.NormalizeAvailability is private while BoF / Multimorphic make it public (the latter is test-driven, the former is a real defect). Tracked as a separate ~3-line PR to keep this one strictly scoped.

Checklist

  • CI is green (build + test + coverage + CodeQL + sanitization)
  • PR title follows the Conventional Commits format above
  • If this is a new architectural decision, an ADR has been added under docs/adr/ — N/A
  • If user-visible behavior changes, README.md and/or docs/ are updated in the same PR — N/A (no behavior change)
  • If a memory in ~/.claude/projects/c--projects-PinballWizard/memory/ is now stale, it has been updated or removed in the same PR
  • No TODO / FIXME / commented-out code committed
  • No new entries in <NoWarn> without a comment explaining why and the removal criterion

Pre-push self-audit

Step 0 — /local-review (qualitative)

  • Ran /local-review and addressed every 🔴 finding before push
  • Local review outcome: 0 🔴 / 1 ⚠️ / 9 categories ✅
    • ⚠️ deferred: pre-existing JJP drift (missing ExtractSlug null-guard; NormalizeAvailability visibility inconsistency). Justification: not introduced by this PR; addressing it would expand a strict-subset adoption PR into a sibling-cleanup PR. Tracked as a separate follow-up (see "Out of Scope").

Step 1 — Mechanical checklist

  • Every new *Options property has at least one real getter call in src/ — N/A (no new options)
  • Sibling-diffed against the closest existing implementation (BoF) — Multimorphic now mirrors the BoF template
  • No bare catch { } — only scoped catch (JsonException) (preserved behavior)
  • New ISourceScraper? — N/A (no new scraper); SourceAliasContractTests still passes
  • Tests assert behavior, not just structure
  • Build is zero-warning
  • git log -1 --format='%an <%ae>' shows personal noreply, not work email

Strict-subset follow-up to PR #42. MultimorphicProductExtractor
deletes its private 140-line JSON-LD walker and routes through the
shared PinballWizard.Infrastructure.Scraping.JsonLd.JsonLdProductParser
that JJP and BoF already consume.

Net: -173 lines / +16 lines in MultimorphicProductExtractor.cs.
Behavior preserved: all 27 Multimorphic tests pass without
modification, including the simultaneous-flat-and-nested-price case
that the shared parser was already designed to cover. Validates the
shared parser against a third real consumer in production code.

BofProductExtractor docstring loses its parenthetical "(when PR #39
lands)" qualifier now that Multimorphic actually consumes the parser.

Pre-push audit: /local-review (0 critical / 1 minor — pre-existing
JJP drift, deferred as out-of-scope) plus 7-item mechanical
checklist (all pass).
@jkeeley2073 jkeeley2073 added the claude-code Generated with Claude Code label May 3, 2026
@jkeeley2073 jkeeley2073 merged commit 024fe6c into main May 3, 2026
5 checks passed
@jkeeley2073 jkeeley2073 deleted the Dev-MultimorphicAdoptsJsonLd branch May 3, 2026 11:23
jkeeley2073 added a commit that referenced this pull request May 3, 2026
Third dedup PR in the series (after #42 JsonLdProductParser and #43
Multimorphic adoption). New shared static helper at
PinballWizard.Infrastructure.Scraping.OpenGraph.OpenGraphExtractor
exposes GetMetaContent(IHtmlDocument doc, string property) which routes
meta[property=] first then meta[name=], returning the trimmed content
attribute. JJP, BoF, Multimorphic each delete the byte-identical
private GetMetaContent and add a using; net -30/+63 across the three
consumers and the helper.

Behavior preserved exactly — including the content="" returns the
empty string semantics that the consumer fallback chains depend on
(the ?? operator only triggers on null; changing empty->null would
silently change downstream fallback ordering).

12 new tests pin every shape: spec form, loose form, both-prefer-property,
missing meta, present-meta-without-content-attribute, whitespace
trimming, empty-content-string-parity (load-bearing), first-match-wins
on duplicates, null/empty/whitespace guards.

Pre-push self-audit: /local-review (0 critical / 3 minor / 7 categories
clean — namespace mild misnomer, doc overpromise, unescaped CSS
interpolation; all documented and acceptable for an internal helper)
plus 7-item mechanical checklist (all pass).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-code Generated with Claude Code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant