Repository navigation
fix: erreur explicite quand une colonne cle est absente d'un dataset - #35
Merged
Merged
Conversation
…(issue #16) Le retour anticipe silencieux (liste tronquee a 6 champs sans coverage ni summary) est remplace par un stop() qui nomme les colonnes cles manquantes et le ou les datasets concernes. Le test qui verrouillait l'ancien comportement est remplace par des expect_error/expect_match couvrant : cle absente des deux cotes, cote candidat seul, sans YAML explicite, et cle multi-colonnes partiellement absente.
There was a problem hiding this comment.
Pull request overview
Ce PR corrige un comportement incorrect de compare_datasets_from_yaml() lorsque la/les colonne(s) clé(s) sont absentes d’un dataset, en remplaçant un retour “dégradé” (liste tronquée) par une erreur explicite, afin d’éviter de casser les consommateurs aval.
Changes:
- Remplacement du
message()+ retour anticipé incomplet par unstop()explicite listant les colonnes clés manquantes et le(s) dataset(s) concerné(s). - Ajout/ajustement de tests testthat couvrant les cas “clé absente” (dont clé multi-colonnes partiellement absente) et nettoyage via
tempfile()+on.exit(unlink(...)). - Mise à jour de la documentation (roxygen + Rd), du NEWS et bump de version en
0.4.9.9000.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-key-parameter.R | Remplace l’ancien test “retour mou” par des assertions sur l’erreur explicite + utilise un YAML temporaire. |
| R/compare_datasets_from_yaml.R | Implémente la détection des clés manquantes via setdiff() par dataset et lève une erreur explicite. |
| NEWS.md | Documente le changement de comportement comme breaking change (référence issue #16). |
| man/compare_datasets_from_yaml.Rd | Synchronise la doc utilisateur avec le nouveau comportement de key. |
| DESCRIPTION | Passe la version en développement (0.4.9.9000). |
Files not reviewed (1)
- man/compare_datasets_from_yaml.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+407
to
+409
| if (!is.null(key)) { | ||
| if (isFALSE(all(key %in% get_col_names(data_reference_p))) | isFALSE(all(key %in% get_col_names(data_candidate_p)))) { | ||
| message("could not find key in both data") | ||
| return( | ||
| list( | ||
| all_passed = FALSE, | ||
| agent = NULL, | ||
| reponse = NULL, | ||
| missing_in_candidate = NULL, | ||
| extra_in_candidate = NULL, | ||
| applied_rules = NULL | ||
| missing_key_ref <- setdiff(key, get_col_names(data_reference_p)) | ||
| missing_key_cand <- setdiff(key, get_col_names(data_candidate_p)) |
Comment on lines
+425
to
+428
| stop(sprintf( | ||
| "Key column(s) not found: %s.", | ||
| paste(details, collapse = "; ") | ||
| ), call. = FALSE) |
Reponse a la review Copilot de la PR : - la cle passee en argument est validee contre les DEUX datasets avant la branche path = NULL, qui atteignait write_rules_template() en premier et levait une erreur ne validant que la reference, dans un format different - une cle vide (character(0)) ou non-character est rejetee proprement au lieu de tomber dans un cross join dplyr deprecie - la validation est factorisee dans validate_comparison_key(), reutilisee par le check tardif qui couvre les cles issues du YAML
Comment on lines
+463
to
+467
| # Re-validated here because the key may come from the YAML rules | ||
| # (defaults$keys), which the argument-level check upstream cannot see. | ||
| validate_comparison_key( | ||
| key, | ||
| ref_cols = get_col_names(data_reference_p), |
Member
Author
There was a problem hiding this comment.
Bien vu sur le fond : c'est l'issue #20 (resolution YAML vs arguments fragile), corrigee dans la PR suivante de la pile via rules$defaults[["keys"]] %||% rules$defaults[["key"]].
Nuance factuelle : sur cette PR telle quelle, la cle YAML est bien recuperee et validee. list(keys = "id")$key renvoie "id" par partial matching de $ (key est un prefixe strict de keys). Le defaut n'est donc pas "never picked up" mais la fragilite de ce match accidentel : il tombe a NULL des qu'un second champ prefixe key* apparait dans le YAML. Le passage a [[ supprime cette dependance.
This was referenced Jul 2, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16
Probleme
Quand la cle etait absente d'un des deux datasets,
compare_datasets_from_yaml()emettait unmessage()vague puis retournait une liste tronquee a 6 champs (sanscoveragenisummary), cassant les consommateurs aval (datadiff_report_html(),res$summary$n_failNULL silencieux,pointblank::all_passed(res$reponse)en erreur).Correctif
stop()explicite nommant la ou les colonnes cles manquantes et le ou les datasets concernes (Key column(s) not found: 'id' missing in data_candidate.)setdiff()par dataset remplace la condition scalaire|(point l.406 de l'issue)@param keydocumente le comportement,man/regenereTests (TDD red-first)
Le test qui verrouillait l'ancien retour mou est remplace par des
expect_error/expect_matchcouvrant : cle absente des deux cotes, cote candidat seul, sans YAML explicite (template auto), cle multi-colonnes partiellement absente. Rouge constate sur l'ancien code (retour liste au lieu d'erreur), vert apres fix.Suite complete : 809 PASS / 0 FAIL (baseline 802 PASS).
Review interne
pr-reviewer : APPROVE, matrice de couverture complete, test de regression verifie discriminant contre la 0.4.9 installee. Nit corrige (
fixed = FALSEredondant). Les YAML ecrits en cwd par les tests voisins relevent de #31.