Skip to content

Separate Nota's tables from the upstream set - #20

Merged
henrikottesorensen merged 2 commits into
mainfrom
chore/drop-stale-test-tables
Oct 9, 2026
Merged

henrikottesorensen merged 2 commits into
mainfrom
chore/drop-stale-test-tables

Conversation

@henrikottesorensen

Copy link
Copy Markdown
Collaborator

Stacked on #9. This PR's base is that branch, so the diff and commit list below are only this change. GitHub will retarget it to main automatically when #9 merges.

Chain: #9 → #10 → #15 → #16

LibLouis.NET.Test/tables/ held thirty of Nota's Danish tables, while LibLouis.NET.Tables copies the upstream set into the same output directory. 22 of the 30 share a file name with an upstream table and differ from it, so which copy a test got depended on MSBuild item ordering — and nothing said so anywhere.

This moves them to nota-tables/, kept separate in the output, and points every test at that path.

Why it matters beyond tidiness

The shadowing has already cost real time. Running upstream's Danish braille specs against the wrapper failed 2,699 forward cases until they were redirected at an unshadowed upstream directory — the tables under test were Nota's, not the ones the specs were written for. With this change tables/ is pure upstream and can be checked against directly.

It also surfaced something worth knowing: Nota's tables are a fork of an older upstream, not a patch on the current one. They add what upstream lacks — the foreign emphasis class behind TypeForm.ForeignLanguage — and are missing what upstream has since fixed, such as the rules removing the space between § and a following number. da-dk-g2.dic diverges by 14,810 lines. Two files exist nowhere upstream, which is why the tests cannot simply use the upstream set.

Rebased onto #9, and why that way round

#9 adds ten test files that build paths from "tables", the directory this renames. Rebasing this onto #9 rather than the reverse means #9 needs no churn: this PR absorbs the mechanical path update across those ten files.

That also unblocked something. The first version of this PR deliberately shipped without a self-containment test, because adding a second test class to the assembly crashed the run on main:

Process terminated. A callback was made on a garbage collected delegate
of type 'LibLouis.NET!LibLouis.NET.NativeMethods+LoggingCallback::Invoke'.

That is #9's log-callback fix, item 5 of the audit — one more allocating test class was enough to trigger a GC and collect the unrooted delegate. On top of #9 it is gone, so both guards are included here.

63 tests pass on the combination.

Guards

  • NotaTablesAreSelfContained — liblouis resolves an include relative to the directory of the table doing the including, so Nota's tables need the seven general upstream tables they include sitting alongside them. Those are copied from the staged upstream set rather than committed, so they cannot drift. The list is explicit in the csproj, and an explicit list goes stale the moment a table gains an include, so this asserts the closure holds.
  • NotaTablesDoNotLeakIntoTheUpstreamDirectory — sharing a file name across the two directories is expected. What must not recur is both sets landing in one directory, so the tables that exist nowhere upstream act as the canary.

Nothing deleted

23 of the 30 are reachable from no test — a stale copy of the set the application ships, drifting independently of both upstream and production. They are kept and documented in nota-tables/README.md rather than deleted; deciding what is canonical isn't this change's business.

🤖 Generated with Claude Code


Recreated from #10 to move the head branch onto Notalib/LibLouis.NET, which GitHub stacked PRs require (stacks cannot span forks).

@nanchen2483

nanchen2483 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor
  • P3 — Conflicting counts for overlapping tables. The PR description says 22 Nota tables share a filename with an upstream table and differ, but the test project comment and the README both say five. Please reconcile the count so the rationale is clear.

🤖 AI-assisted review: This PR has been reviewed with AI assistance.

henrikottesorensen added a commit to henrikottesorensen/LibLouis.NET that referenced this pull request Oct 9, 2026
Review feedback on Notalib#20: the PR description said 22 Nota tables share a
file name with an upstream table and differ from it, while the csproj
comment and nota-tables/README.md said five.

Both were true of different sets. Against the liblouis 3.33.0 tables,
22 of the 30 files collide and every one of them differs; five of those
are among the seven files the tests actually load (the other two,
da-dk-g16-markers.ctb and da-dk-braillo.dis, exist nowhere upstream).
Say both, so the numbers stop contradicting each other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@henrikottesorensen

Copy link
Copy Markdown
Collaborator Author

P3 — Conflicting counts for overlapping tables.

Fixed in 01c50a0. Both numbers were true of different sets. Compared against the liblouis 3.33.0 tables, 22 of the 30 Nota files share a name with an upstream table and every one of them differs. Five of those are among the seven files the tests actually load (the other two, da-dk-g16-markers.ctb and da-dk-braillo.dis, have no upstream counterpart). The README and csproj comment now say both.

While restacking I also found this branch had been cut from #18 rather than #19. Merging both would have left #19's SpacingTests pointing at the old tables/ directory. The branch is now rebased onto #19, and those paths are updated in the move commit.

henrikottesorensen added a commit to henrikottesorensen/LibLouis.NET that referenced this pull request Oct 9, 2026
Review feedback on Notalib#20: the PR description said 22 Nota tables share a
file name with an upstream table and differ from it, while the csproj
comment and nota-tables/README.md said five.

Both were true of different sets. Against the liblouis 3.33.0 tables,
22 of the 30 files collide and every one of them differs; five of those
are among the seven files the tests actually load (the other two,
da-dk-g16-markers.ctb and da-dk-braillo.dis, exist nowhere upstream).
Say both, so the numbers stop contradicting each other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nanchen2483
nanchen2483 force-pushed the chore/drop-stale-test-tables branch from db3b36e to 7472694 Compare October 9, 2026 13:19
henrikottesorensen added a commit that referenced this pull request Oct 9, 2026
Review feedback on #20: the PR description said 22 Nota tables share a
file name with an upstream table and differ from it, while the csproj
comment and nota-tables/README.md said five.

Both were true of different sets. Against the liblouis 3.33.0 tables,
22 of the 30 files collide and every one of them differs; five of those
are among the seven files the tests actually load (the other two,
da-dk-g16-markers.ctb and da-dk-braillo.dis, exist nowhere upstream).
Say both, so the numbers stop contradicting each other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@henrikottesorensen
henrikottesorensen force-pushed the chore/drop-stale-test-tables branch from 7472694 to 349c223 Compare October 9, 2026 13:49
@henrikottesorensen

Copy link
Copy Markdown
Collaborator Author

Correction to my comment above: the commit hash has changed. After restacking onto the current main, the count fix is 349c223, and the SpacingTests path change is in 22d3cab. The counts still hold against liblouis 3.38.0's tables (22 of 30, five of the seven loaded).

Base automatically changed from fix/spacing-output-buffer to main October 9, 2026 13:51
@henrikottesorensen
henrikottesorensen force-pushed the chore/drop-stale-test-tables branch from 349c223 to 5b18289 Compare October 9, 2026 13:51
henrikosorensen and others added 2 commits October 9, 2026 16:13
LibLouis.NET.Test/tables held thirty of Nota's Danish tables while
LibLouis.NET.Tables copies the upstream set into the same output
directory. Twenty two of the thirty share a file name with an upstream
table and differ from it, so which copy a test got depended on MSBuild
item ordering, and nothing said so anywhere.

Moves them to nota-tables/, kept separate in the output, and points every
test at that path. Each test now states which set it means, and the
upstream tables are no longer shadowed, which is what lets upstream's
braille specs be checked against them.

Nota's tables include seven general upstream tables, and liblouis
resolves an include relative to the directory of the table doing the
including, so those are copied in alongside. They come from the staged
upstream set rather than being committed, so they cannot drift from it.
NotaTablesAreSelfContained asserts that list stays complete, since an
explicit list goes stale the moment a table gains an include.

The second guard checks that none of Nota's tables reach tables/. Sharing
a file name across the two directories is expected - most of Nota's are
forks of an upstream table of the same name - so the tables that exist
nowhere upstream are the canary: if one appears in tables/, the two sets
are being copied to the same place again.

No table content changes. Twenty three of the thirty are reachable from
no test - they are a stale copy of the set the application ships - but
they are kept and documented rather than deleted, because deciding what
is canonical is not this change's business.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on #20: the PR description said 22 Nota tables share a
file name with an upstream table and differ from it, while the csproj
comment and nota-tables/README.md said five.

Both were true of different sets. Against the liblouis 3.33.0 tables,
22 of the 30 files collide and every one of them differs; five of those
are among the seven files the tests actually load (the other two,
da-dk-g16-markers.ctb and da-dk-braillo.dis, exist nowhere upstream).
Say both, so the numbers stop contradicting each other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@henrikottesorensen
henrikottesorensen force-pushed the chore/drop-stale-test-tables branch from 5b18289 to d579446 Compare October 9, 2026 14:13
@henrikottesorensen
henrikottesorensen merged commit 688caf0 into main Oct 9, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants