Repository navigation
fix: report.R durci - memoisation partagee, eval_error propage, constantes centralisees - #45
Merged
Conversation
…antes centralisees (issue #29) - datadiff_report_html() passe par datadiff_render_report() : lit le cache alimente par print() et l'alimente en retour (plus de double construction agent + rapport) - un eval_error sur l'agent reel (n_failed NA) prend la branche reelle du build (le garde any(n_failed > 0) seul routait silencieusement vers la branche synthetique all-pass) et survit au bloc vectorise (qui forcait eval_error/eval_warning a FALSE sur toutes les lignes) - bloc mort supprime dans build_report_agent : les injections de compteurs par ligne du else etaient integralement reecrites par le bloc vectorise authoritative-from-coverage - R/constants.R : suffixes __ok/__eq, prefixes __missing_col_/ __type_mismatch_ et defauts warn_at/stop_at definis une fois et partages par producteurs, consommateurs du verdict et mapping du rapport ; helpers datadiff_ok_col()/datadiff_eq_col()
There was a problem hiding this comment.
Pull request overview
This PR hardens datadiff’s report rendering path by reusing the existing lazy-report cache (shared with print()), correctly propagating pointblank evaluation errors into the rebuilt report agent, and centralizing internal naming/threshold constants to prevent silent mapping drift (per issue #29).
Changes:
datadiff_report_html()now reuses the memoized report viadatadiff_render_report()whenres$reponseis adatadiff_report, avoiding unnecessary agent/report rebuilds.build_report_agent()no longer masks realeval_error/eval_warningfor mapped steps and avoids incorrectly routing “pure eval_error” agents to the synthetic all-pass branch.- Introduces
R/constants.Rto centralize__ok/__eqand dummy-column naming conventions plus default warn/stop thresholds, and updates producers/consumers to use these helpers/constants.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-report-robustness.R | Adds regression tests for shared memoization and eval_error propagation in the report agent. |
| R/tolerance.R | Switches __ok/__eq column naming to centralized helpers (datadiff_ok_col(), datadiff_eq_col()). |
| R/report.R | Reuses the lazy-report cache in datadiff_report_html(), preserves mapped step eval flags, removes dead count-injection code, and uses centralized defaults. |
| R/pointblank_setup.R | Uses centralized dummy-column prefixes and equality/tolerance column helpers when building pointblank agents. |
| R/fast_path.R | Uses centralized helpers for boolean column lookup and improves related internal error messages accordingly. |
| R/constants.R | Adds shared internal constants and helpers for naming conventions and default warn/stop thresholds. |
| R/compare_datasets_from_yaml.R | Aligns default threshold literal formatting and switches internal suffix usage to shared constants/helpers. |
| NEWS.md | Documents the robustness changes and the new centralized constants (issue #29). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 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 #29
Les 4 points de l'issue
datadiff_report_html()reconstruisait agent + rapport a chaque appel. Il passe pardatadiff_render_report(): lit le cache alimente parprint()et l'alimente en retour. Fallback synthetique one-shot pour unresassemble a la main (sans wrapper).any(n_failed > 0)routait un agent entierement en erreur d'evaluation (n_failed NA partout) vers la branche synthetique all-pass (extraits perdus), et le bloc vectorise forcaiteval_error/eval_warninga FALSE. Le garde ajoute|| any(eval_error)et les flags reels des steps mappes survivent au bloc vectorise. Note en commentaire : les erreurs d'evaluation des steps structurels ne sont volontairement pas surfacees par colonne.R/constants.Rdefinit une fois les suffixes__ok/__eq, prefixes__missing_col_/__type_mismatch_(+ helpersdatadiff_ok_col()/datadiff_eq_col()) et les defauts warn/stop - partages par producteurs, consommateurs du verdict et mapping du rapport. Une derive unilaterale ne peut plus casser le mapping silencieusement. Le litteral public 1e-14 reste dans la signature (14 zeros remplaces) pour ne pas fuiter un symbole interne dans la section Usage du man.Tests (TDD red-first)
test-report-robustness.R: contrat de cache (null avant, alimente par le premier datadiff_report_html, second appel identique au cache), eval_error sur step mappe survit, agent tout-NA-eval_error prend la branche reelle. 4 rouges avant fix.Suite complete : 913 PASS / 0 FAIL sur la pile.
Review interne
pr-reviewer : APPROVE (mapping match() 1:1 verifie sur agent live, sweep des constantes verifie par grep, memoisation contenu-identique). Nits appliques : messages d'erreur de fast_path composes via les helpers de constantes, commentaire sur les eval_error structurels.