Let a second call refine the config verbs instead of replacing it (#119) - #121
Merged
Conversation
A second clin_row_height() replaced the first, so anything the earlier call had set reverted to its default with no error or warning. The natural pattern - a house wide call, then a per table exception - was therefore impossible without restating every argument, and getting it wrong rendered a 3.189% pixel difference silently. The reporter's argument for merging is right and it is a cleaner case than the composition question on #113: the state being merged is clinify's own config, where "not specified" is unambiguous, so there is nothing to guess. It was not one function. clin_header_pad() lost `above` the same way, and clin_spanner_rule() lost `border`. All three now fold only the arguments a call actually names into what was already there. clin_table_align() carries a single value, so replacing was already correct there. Two things the fix turns on: Arguments with a default cannot be recognised by being NULL - `rule` and `border` are always populated - so the merge keys off missing() rather than is.null(). And missing() has to be captured before match.arg() runs, because assigning to a formal clears its missing status. Without that, a second call still reset `rule` to "atleast" while looking correct everywhere else. There is a test for exactly that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 #119.
Your argument for merging is right, and it is a cleaner case than #113 for exactly the reason you gave: the state being merged is clinify's own config, where "not specified" is unambiguous, so there is nothing to guess.
It was not one function
Checking the siblings before fixing the reported one:
clin_row_height()title = 11.4bodylost — as reportedclin_header_pad()rule_to_body = 6abovelostclin_spanner_rule()rows = 1borderlost (a dashed pen reverted to the default)clin_table_align()All three now fold only the arguments a call actually names into what was already there. Fixing just the reported one would have left two known silent-data-loss bugs of the same shape.
Two things the fix turns on
Arguments with a default cannot be recognised by being NULL.
ruleandborderare always populated, so a naiveis.null()merge would still silently reset them. The merge keys offmissing().missing()has to be captured beforematch.arg()runs — assigning to a formal clears its missing status. Without that the fix looked correct everywhere except that a second call still resetruleto"atleast". Caught it in verification, and there is a test pinning it:On
rows, and why merging is still flatclin_header_pad()andclin_spanner_rule()both carry arowsfield that qualifies the values. Merging is field-wise, so a later call namingrowsreplaces the earlierrowsfor the whole setting rather than layering a second instruction. Documented as such. Layering different values on different header rows is the per-row vector formabove = c(18, 34)from #113, which is a better fit than two calls anyway — I did not want two ways to express the same thing with different semantics.Verification
devtools::check()— 0 errors, 0 warnings, and theunable to verify current timeNOTE, which is the no-network note this repo'scran-comments.mddocuments as local-only.object_usage_linterfalse positive for the shared helper, which single-filelint()cannot resolve and R CMD check's own pass does not flag.rulesurviving and being overridable, and a bareclin_spanner_rule()still ruling whileborder = FALSEstill does not.One correction to my own verification: I initially asserted
isFALSE()onborder = FALSEand a probe printedFALSEwhere I expectedTRUE, which I glossed over before the test caught it.border = FALSEis normalised into a zero-widthfp_borderrather than kept as a logical — a sensible representation, and the assertion now checkswidth == 0/style == "none".Note
#120 is being investigated separately.
DESCRIPTIONleft at 0.4.0 with the entry under# clinify (development version).🤖 Generated with Claude Code