Skip to content

fix(rules): restore pk column to Rules CSV (follow-up to #228) - #233

Merged
jsboige merged 1 commit into
masterfrom
fix/rules-csv-restore-pk-column
Apr 24, 2026
Merged

fix(rules): restore pk column to Rules CSV (follow-up to #228)#233
jsboige merged 1 commit into
masterfrom
fix/rules-csv-restore-pk-column

Conversation

@jsboige

@jsboige jsboige commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Problem

PR #228 merged with DatasetUpdaterRootConfig.cs referencing PrimaryField = "pk" for the Rules PT translation config, but the accompanying CSV change (pk column addition with Rules_01..Rules_18 prefixes) was lost during an earlier rebase of that PR branch.

Master's Cards/Rules/Argumentum Rules - Cards.csv currently has headers:
```
Text,Text_en,Text_ru,Text_pt,print_and_play
```

No `pk` column. The config references it anyway:

```csharp
// DatasetUpdaterRootConfig.cs:675
PrimaryField = "pk",
```

Result: if someone sets Enabled = true on the Rules PT config and runs the pipeline, DatasetUpdater will fail to identify rows (no primary key column to match).

Fix

Restore the exact pk column that was in the original PR #228 commit (`a99f228a`), matching what Rule.cs expects via its Pk property + ClassMap.

Header becomes:
```
pk,Text,Text_en,Text_ru,Text_pt,print_and_play
```

Rows get prefixed Rules_01..Rules_18. 18 data rows — matches the game's rule-card structure (École des Menteurs, Bingo, Parlote Coinchée variants).

Scope

  • One file changed: Cards/Rules/Argumentum Rules - Cards.csv
  • No code changesRule.cs already has .Optional() on the Pk mapping (robust to reads)
  • Config still Enabled = false — this is purely a latent-bug fix with zero runtime behavior change for anyone who hasn't enabled the config

Diff

19 insertions + 19 deletions (just the row-level boundaries where pk is prepended):

```
-Text,Text_en,Text_ru,Text_pt,print_and_play
+pk,Text,Text_en,Text_ru,Text_pt,print_and_play
-"# Argumentum
+Rules_01,"# Argumentum
...
```

Validation

  • ✅ Build: 0 errors, 17 warnings (all pre-existing)
  • ✅ Tests: 88 pass, 1 skip (Freeplane GUI — interactive-only), 0 fail

Test plan

  • Verify Rules CSV reads cleanly via existing RuleClassMap (Pk mapping with .Optional() already in place)
  • Manual: temporarily Enabled = true on Rules PT config with TakeChunkNb = 1 and confirm DatasetUpdater identifies rows by pk (full end-to-end run deferred to translation campaign with OpenAI key)

Related

🤖 Generated with Claude Code

PR #228 merged with DatasetUpdaterRootConfig.cs referencing `PrimaryField = "pk"`
for the Rules PT translation config, but the accompanying Rules CSV change (pk
column addition with Rules_01..Rules_18 prefixes) was lost during an earlier
rebase of that PR. Master's Rules CSV therefore has no `pk` column, which would
cause the DatasetUpdater to fail at runtime if someone enables the Rules PT config.

This fix restores the exact `pk` column that was present in the original PR #228
commit, matching what Rule.cs expects via its Pk property + ClassMap. The config
is still Enabled=false by default, so this is purely a latent-bug fix — no
behavior changes for anyone who hasn't enabled the config.

Changes:
- Cards/Rules/Argumentum Rules - Cards.csv: prepend `pk` column header +
  `Rules_01` through `Rules_18` row prefixes (18 data rows, matches the 18
  rule cards in the game's print&play structure).

Validation:
- Build: 0 errors
- Tests: 88 pass, 1 skip (Freeplane GUI), 0 fail

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jsboige

jsboige commented Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

Review — self-review before merge

Contexte

Fix chirurgical pour un bug latent découvert après merge de #228. Je l'ai documenté en détail dans la PR body. Rappel rapide :

  • DatasetUpdaterRootConfig.cs:675 sur master a PrimaryField = "pk" pour la config Rules PT
  • Cards/Rules/Argumentum Rules - Cards.csv sur master n'a PAS de colonne pk (perdue lors d'un rebase de feat(dataset-updater): add Rules PT translation config #228)
  • → Si quelqu'un Enabled = true sur la config, DatasetUpdater ne trouverait pas de clé primaire → crash

Audit du fix

Item Vérification
Colonne pk ajoutée pk,Text,Text_en,Text_ru,Text_pt,print_and_play
Préfixes Rules_NN ✅ Rules_01..Rules_18 (18 lignes = 18 cartes règles)
Correspond au commit original #228 ✅ Restauration exacte du contenu a99f228
Rule.cs côté code ✅ Pk mapping déjà présent sur master avec .Optional()
Impact runtime tant que Enabled=false ✅ Zéro (config désactivée par défaut)

Validation

  • ✅ Build: 0 erreurs
  • ✅ Tests: 88 pass / 1 skip (Freeplane GUI) / 0 fail
  • ✅ GitGuardian Security Checks: SUCCESS
  • ✅ Mergeable: CLEAN
  • ✅ Diff minimal: 1 fichier, +19/-19 (prépendage de colonne uniquement)

Conclusion

Fix safe, self-contained, resolves a latent bug. Prêt à merger.

@jsboige
jsboige merged commit e3a66fd into master Apr 24, 2026
1 check passed
@jsboige
jsboige deleted the fix/rules-csv-restore-pk-column branch April 24, 2026 00:57
@clusterManager-Myia

Copy link
Copy Markdown
Collaborator

Post-hoc review — PR #233 fix(rules): restore pk column to Rules CSV (follow-up to #228)

Verdict: ✅ LGTM — necessary follow-up

This PR restores the pk column to the Rules CSV that was referenced by PR #228's DatasetUpdater config but missing from the actual CSV file.

Observations:

  • PK values follow the Rules_01, Rules_02, ... pattern — consistent and clear.
  • All 18+ rows get proper PK assignment.
  • No content changes to any text columns — purely structural.
  • This is the kind of follow-up that highlights the value of post-merge review: the CSV config was deployed before the CSV schema was updated.

No regressions. Correct fix.

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.

2 participants