fix: setup_pointblank_agent - cols_reference deprecie, steps egalite sur booleen derive#52
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses API and performance issues around setup_pointblank_agent() by deprecating an unused argument, aligning equality validation with the package’s shared boolean-based semantics, and making the documentation/examples executable and representative (per issue #25).
Changes:
- Deprecate
cols_reference(kept in position for compatibility) with a warning when provided, and remove dead locals at the caller. - Switch equality validation steps to validate
<col>__eqbooleans (deriving them for local tables; enforcing presence for lazy tables), avoiding embedding full reference vectors in validation steps. - Update tests, NEWS, and generated docs to reflect the new contract and example usage.
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-pointblank-setup.R |
Updates calls to named args and adds tests for deprecation + NA/equality semantics. |
R/pointblank_setup.R |
Implements cols_reference deprecation and boolean-based equality step behavior; updates roxygen docs/example. |
R/compare_datasets_from_yaml.R |
Removes dead locals and updates the setup_pointblank_agent() call to avoid deprecated positional usage. |
NEWS.md |
Documents the deprecation and equality-step behavior change as a bug fix. |
man/setup_pointblank_agent.Rd |
Updates generated usage/details/examples to match the new contract. |
Files not reviewed (1)
- man/setup_pointblank_agent.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+170
to
+175
| # Omitting it works and produces the same agent shape | ||
| agent <- setup_pointblank_agent(cmp, common_cols = "a", | ||
| tol_cols = character(0), row_validation_info = row_info, | ||
| ref_suffix = "__reference", warn_at = 0.1, stop_at = 0.1, | ||
| label = "x", na_equal = TRUE) | ||
| expect_s3_class(agent, "ptblank_agent") |
Member
Author
There was a problem hiding this comment.
Valides tous les deux, appliques : expect_no_warning sur le cas omis, et la doc precise que seul un non-NULL declenche le warning (un NULL explicite est silencieux).
Comment on lines
+14
to
+16
| #' @param cols_reference Deprecated and unused; supplying it raises a warning | ||
| #' and it will be removed in a future release | ||
| #' @param common_cols Character vector of equality columns to validate (the |
…sur booleen derive (issue #25) - cols_reference jamais lu depuis son introduction : NULL par defaut + warning de depreciation quand fourni ; l'appelant pipeline passe en arguments nommes sans lui ; variables mortes cols_reference/ cols_candidate supprimees de l'appelant - les steps d'egalite locaux valident un booleen __eq derive en amont de create_agent (semantique NA partagee : unilateral echoue, bilateral suit na_equal) au lieu d'embarquer le vecteur de reference entier dans chaque step (O(lignes) par step, serialise avec le rapport, na_pass incapable d'exprimer la distinction) ; un cmp lazy sans __eq precalcule recoit une erreur de contrat explicite - exemple roxygen executable et representatif (il ciblait b__ok inexistant : l'agent produit echouait a l'interrogation) - get_col_names hisse hors boucle, calcule apres toutes les mutations - tests : depreciation, semantique NA des appels directs (rouge avant), appels legacy du fichier de tests passes en arguments nommes
VincentGuyader
force-pushed
the
fix/issue-25-setup-agent
branch
from
July 3, 2026 10:17
b9ebd6a to
e30f8c3
Compare
This was referenced Jul 3, 2026
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 #25
Les 5 points de l'issue
cols_reference(2e position, documente) jamais lu depuis son introduction (verifie par grep). Depreciation propre :NULLpar defaut + warning quand fourni, position conservee (pas de decalage silencieux d'arguments, verifie par la review) ; variables mortescols_reference/cols_candidatesupprimees de l'appelant.__eqderive en amont decreate_agentvia le kernel partageeq_col_bool- ce qui aligne aussi la semantique NA des appels directs (unilateral echoue) quena_passne peut pas exprimer.b__okinexistant, l'agent produit echouait a l'interrogation).__eqrecoit une erreur de contrat explicite.get_col_nameshisse hors boucle, apres toutes les mutations ; doc decommon_colsprecise que le pipeline ne passe que les colonnes en echec.Tests (TDD red-first)
Warning de depreciation, semantique NA des appels directs (rouge avant), appels legacy du fichier passes en arguments nommes (zero warning residuel).
Suite complete : 1023 PASS / 0 FAIL sur la pile. Check --as-cran 0/0.
Review interne
pr-reviewer : APPROVE (pas de risque de decalage d'arguments, mutation avant create_agent donc agent$tbl coherent).