Using BaseTextParser for every Text Parser

[Flamifly]

• Instead of PlainTextParser the BaseTextParser will be used since none of the sub classes uses the functionality of the PlainTextParser.
• I also added support for Streams for every Text Parser
• I moved the Office & PDF Parsers to it's own Directories

[github-actions]

Test Results

142 tests  ±0   141 ✅ ±0   7s ⏱️ +3s
  2 suites ±0     1 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit d06ba43. ± Comparison against base commit 99ca266.

♻️ This comment has been updated with latest results.

[Flamifly]

/oc help review this PR

[opencode-agent]

Here's my review of PR packages → master by @Flamifly:

Summary

Consolidates text parsers under BaseTextParser, adds stream support, and reorganizes Office/PDF/RTF parsers into subdirectories. All 141 tests pass.

Positive

• Reduced duplication: Previously each parser had its own Parse() with try/finally + disposal boilerplate. BaseTextParser.Parse() centralizes that, subclasses only implement ParseText().
• Stream support: Parsers that only supported file paths (PowerPoint, Word, PDF, RTF, EPUB) now work with streams via ParserContext's IsStreamContext.
• Directory structure: Parsers/Office/, Parsers/PDF/, Parsers/RTF/ improves organization.

Issues

1. out → ref change on ParseText (commit 4f76f64): Changed from out IDisposable? to ref IDisposable?. The ref is unnecessary since the base always passes a fresh null variable. out is semantically correct here — the value flows out, not in/out. This also forces implementors to explicitly set disposable = null (e.g., ExcelTextParser:16, PowerPoint2007TextParser:20) even though the base already initializes it. Use out instead.

2. Word2007TextParser redundant assignment (Parsers/Office/Word2007TextParser.cs:13): Context = context; is redundant — BaseTextParser(Context) already does this. Pre-existing, carried over from old code.

3. PowerPoint2007TextParser dead code (Parsers/Office/PowerPoint2007TextParser.cs:41-42):

   string relId = (slideIds[index] as SlideId).RelationshipId;
   relId = (slideIds[index] as SlideId).RelationshipId;  // overwrites immediately

   First assignment is dead. Pre-existing.

4. PowerPoint2007TextParser null-check bug (Parsers/Office/PowerPoint2007TextParser.cs:60):

   ArgumentNullException.ThrowIfNull("slidePart");  // passes string, not parameter

   Should be ThrowIfNull(slidePart). As written it never throws because "slidePart" is never null. Pre-existing.

5. #nullable disable inconsistency: BaseTextParser enables nullable context internally but the PR's new parsers (PDFTextParser, RTFTextParser, Word2007TextParser, PowerPoint2007TextParser) don't use nullable annotations for disposable. Consider aligning.

Verdict

Approve with minor suggestions. Changes are net positive — consolidation is well-motivated, stream support is valuable, and all tests pass. Issues 2–5 are pre-existing and don't block merge. Issue 1 (ref vs out) is a style preference but worth fixing for consistency.

opencode session  |  github run

[Flamifly]

ParseText passes always a Stream to avoid to have the Stream handling in the Parsers and have one thing to parse

[Flamifly]

/oc help review this PR

[opencode-agent]

Review: PR packages → master by @Flamifly

Consolidates text parsers under BaseTextParser (centralizing stream open/dispose), adds stream support, and reorganizes Office/PDF/RTF parsers into subdirectories. All 141 tests pass.

Positive

• Clean base class pattern: BaseTextParser.Parse() handles stream creation and disposal; subclasses implement ParseText(Stream) with no boilerplate.
• Stream support: PowerPoint, Word, PDF, RTF, EPUB now work with streams via ParserContext.
• Directory organization: Parsers/Office/, Parsers/PDF/, Parsers/RTF/ is a clear improvement.

Bug found

ExcelTextParser ignores the stream parameter (Parsers/Spreadsheet/ExcelTextParser.cs:27-33):

// Current (wrong): re-reads from Context instead of using the stream already opened by BaseTextParser
if (Context.IsStreamContext)
    workbook = WorkbookFactory.Create(Context.Stream);
else
    workbook = WorkbookFactory.Create(Context.Path);

Should simply be WorkbookFactory.Create(stream). This defeats the purpose of BaseTextParser — the base opens and disposes a stream that is never used. VCardTextParser and others correctly use the passed stream parameter, so this looks like an oversight.

Pre-existing (carried over, not blocking)

1. PowerPoint2007TextParser:45 — relId assigned twice, first assignment dead.
2. PowerPoint2007TextParser:64 — ArgumentNullException.ThrowIfNull("slidePart") passes a string literal; should be ThrowIfNull(slidePart). Never throws as written.
3. Word2007TextParser:13 — Context = context is redundant (base constructor does this).
4. Nullability inconsistency — BaseTextParser uses #nullable enable/disable but subclasses don't annotate.

---

Verdict: Approve — the changes are net positive and well-motivated. The ExcelTextParser bug should be fixed before merge (single-line change). Everything else is minor/style.

opencode session  |  github run

[Flamifly]

• Removed second relId assignment
• Fixed ArgumentNullException.ThrowIfNull
• Removed Context assignment
• Removed nullability from BaseTextParser
• Fixed ExcelTextParser bug

[Flamifly]

/oc help review this PR

[opencode-agent]

All issues from prior reviews have been addressed:

Issue	Status
ref vs out on ParseText	Resolved — now passes Stream directly, cleaner design
ExcelTextParser ignoring stream parameter	Fixed — uses WorkbookFactory.Create(stream)
PowerPoint2007TextParser dead relId assignment	Removed
ArgumentNullException.ThrowIfNull("slidePart")	Fixed — properly uses ThrowIfNull(slidePart, ...)
Word2007TextParser redundant Context = context	Removed
Nullability inconsistency	Resolved — no #nullable directives

The consolidation under BaseTextParser is well-executed:

• BaseTextParser.Parse() handles stream opening, validation, and disposal — subclasses implement a clean ParseText(Stream)
• Stream support works uniformly across Word, PowerPoint, PDF, RTF, EPUB, ODF, ZIP, VCard, email, and plain text
• Directory reorg (Parsers/Office/, Parsers/PDF/, Parsers/RTF/) is an organizational improvement

One minor nit: PDFTextParser doesn't call ValidateContext() or ThrowIfProtected() — the old PDFTextParser did check for password protection. The BaseTextParser.Parse() calls ValidateContext() but Utility.ThrowIfProtected(Context) is not in the base — each subclass that needs it must override ValidateContext. This means PDFs with password protection won't be caught early (PdfPig will throw its own error when opening a protected stream, so it's not a silent failure, just a different error message). Worth adding an override similar to:

internal override void ValidateContext()
{
    base.ValidateContext();
    Utility.ThrowIfProtected(Context);
}

Verdict: Approve. The code is clean, tests pass, and the refactoring is well-motivated. The PDF protection check is a minor polish item, not a blocker.

opencode session  |  github run

[Flamifly]

The Comment about PDFTextParser is wrong, it overwrites the ValidateContext Method

internal override void ValidateContext()
{
	base.ValidateContext();
	Utility.ThrowIfProtected(Context);
}

[Flamifly]

If you are fine with it @tonyqus I would merge it