Skip to content

[FIX] Accept remaining legacy option spellings in the argument normalizer - #2376

Closed
pantha704 wants to merge 1 commit into
CCExtractor:masterfrom
pantha704:fix/legacy-option-normalization
Closed

[FIX] Accept remaining legacy option spellings in the argument normalizer#2376
pantha704 wants to merge 1 commit into
CCExtractor:masterfrom
pantha704:fix/legacy-option-normalization

Conversation

@pantha704

@pantha704 pantha704 commented Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

[FIX]

Fixes #2367

In raising this pull request, I confirm the following (please check boxes):

Reason for this PR:

  • This PR adds new functionality.
  • This PR fixes a bug that I have personally experienced or that a real user has reported and for which a sample exists.
  • This PR is porting code from C to Rust.

Sanity check:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • If the PR adds new functionality, I've added it to the changelog. If it's just a bug fix, I have NOT added it to the changelog.
  • I am NOT adding new C code unless it's to fix an existing, reproducible bug.

Repro instructions:

sample.scc (from #2367):

Scenarist_SCC V1.0

00:00:01:00	9420 9420 94ae 94ae 9452 9452 97a2 97a2 c845 4c4c 4f80 942f 942f

00:00:03:00	942c 942c

00:00:04:00	9420 9420 94ae 94ae 9452 9452 97a2 97a2 574f 524c c480 942f 942f

00:00:06:00	942c 942c

Before (current master):

$ ccextractor -UCLA sample.scc
Unknown Argument
error: unexpected argument '--UCLA' found

$ ccextractor -608 sample.scc
error: unexpected argument '-6' found

$ ccextractor -parsepat sample.scc
error: unexpected argument '--parsepat' found

After (this branch, same commands):

$ ccextractor -UCLA sample.scc -o out.srt

1
00:00:01,001 --> 00:00:02,999
      HELLO

2
00:00:04,001 --> 00:00:05,999
      WORLD

$ ccextractor -608 sample.scc -o out.srt    # same result

All the spellings listed in #2367 now parse: -608, -708, -90090, -UCLA, -CC2, -LF, -DF, -parsepat, -parsepmt. (-CC2 parses and targets channel 2 as before; the sample only has channel 1, so it produces no captions.)

What this changes: normalize_legacy_option() in src/rust/src/lib.rs now maps the remaining legacy spellings to their canonical long options: the digit-leading flags (-608, -708, -90090) and the case variants (-UCLA, -CC2, -LF, -DF, -parsepat, -parsepmt). Explicit mappings are used, in the same style as the existing -1 / -2 / -12 handling, so values such as --delay -500 are never rewritten.

Tests: cargo test test_normalize_legacy_option -> 8 passed; full workspace cargo test -> 407 passed.

@pantha704 pantha704 mentioned this pull request Oct 15, 2026
Closed
@ccextractor-bot

ccextractor-bot commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 32/34

66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.

Compared with the tip of master — test 9642, commit 97a0d24:

  • 0 pass there and fail here
  • 0 fail there and pass here
  • 0 fail on both, with different output
  • 66 fail on both, byte for byte the same

Compared with the commit this branch was cut from: the same run as the tip of master (test 9642), so the comparison above already covers it.


No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch.

@ccextractor-bot

ccextractor-bot commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on windows. 170/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 0/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 32/34

67 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.

Compared with the tip of master — test 9643, commit 97a0d24:

  • 1 pass there and fail here
  • 0 fail there and pass here
  • 3 fail on both, with different output
  • 63 fail on both, byte for byte the same

Pass there, fail here:

Fail on both but produce different output — behaviour moved even though the verdict did not:

Compared with the commit this branch was cut from: the same run as the tip of master (test 9643), so the comparison above already covers it.


This branch changes the behaviour of 4 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides.

@pantha704 pantha704 mentioned this pull request Oct 16, 2026
Merged
@muntasir-396 muntasir-396 mentioned this pull request Oct 17, 2026
Open
5 of 7 tasks
kaihere14
kaihere14 suggested changes Oct 17, 2026
View reviewed changes

@kaihere14 kaihere14 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up #2367. I think this one can be closed as a duplicate.

#2368 already fixes the nine spellings from the issue, and #2370 fixes --delay -500 / --delay -12. I merged the two locally: clean merge, cargo test passes, and all nine spellings parse to the same args as their new names.

Your PR does the same nine spellings through explicit maps, which is a nice simple approach, but it doesn't cover the delay bug. So nothing from this PR would be left after #2368 and #2370 land.

Thanks again for the work, and feel free to review #2368 or #2370 if you want to look at the code.

last call will be by the maintainer , please take a look @cfsmp3

@cfsmp3

cfsmp3 commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for this — the same nine spellings were fixed by #2368, merged on Oct 17, and #2383 has since covered the remaining legacy options. All the forms this PR handles already work on master, so I'm closing it as superseded.

@cfsmp3 cfsmp3 closed this Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@kaihere14 kaihere14 kaihere14 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Legacy options -608, -708, -90090, -UCLA, -CC2, -LF, -DF, -parsepat, -parsepmt still rejected after #1856

4 participants

@pantha704 @ccextractor-bot @cfsmp3 @kaihere14