API review: group sheet view settings, tidy docs and argument order - #127
Merged
Conversation
xl_sheet() had grown to 23 arguments, nine of them the tab and opening-view settings added in Phase 4. xl_page_setup() already established the pattern for this kind of cluster, so the view settings now follow it: xl_sheet(view = xl_sheet_view(...)). The signature drops to 15 arguments. freeze deliberately stays on xl_sheet(). It is the single most common worksheet option -- keeping the header row visible -- and making the common case wordier in order to tidy the rare ones would be a poor trade. split, its rarely-used divider variant, does move, and the two remain mutually exclusive with an error that now names both spellings. Done before adding data validation, so that argument lands in a function that is not already overloaded. Nothing here has been released, so no user code breaks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uments Follow-up to the xl_sheet_view() regrouping, from an API review before the next phase widens things further. The three cell shorthands each repeated the same factor-to-character coercion and a stopifnot() whose failure printed an expression rather than a name. They now share .as_character_arg(), which reports the argument and the class it got. write_xlsx()'s format_headers documentation still described the header as "centered and bold". That stopped being true when the header format became configurable through xl_properties(header_format =), so it now says what the argument actually does and points at the setting. xl_workbook() inherits both that and col_names rather than carrying its own paraphrase. xl_hyperlink(name =), xl_hyperlink_cell(value =) and xl_cell_general(value =) all mean the display text shown instead of a URL; the docs now say so, since the differing names are only an accident of xl_hyperlink() mirroring Excel's HYPERLINK() argument. Two signatures now lead with their most probable argument: xl_font() with bold and italic rather than name and size, and xl_comment() with author before format. Every call site names its arguments past the first, so nothing positional depended on the old order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sistency xl_hyperlink() called its display-text argument `name` while xl_hyperlink_cell() and xl_cell_general() called the same thing `value`. It is now `value`, taking the argument position `name` used to occupy, so code that passed the display text positionally is unaffected. `name` is retained, warns and points at `value`, and giving both is an error since they mean the same thing. Reverts the xl_comment() reorder from the previous commit. Shared arguments should keep the same relative order across functions so that reading one signature teaches the others, and that outranks putting each function's most probable argument first. Across xl_formula(), xl_rich_run(), xl_merge() and xl_hyperlink_cell(), `format` immediately follows the content arguments; putting `author` in between broke that idiom. xl_font() keeps its reordering, whose arguments are shared with nothing. The rule now has a mechanical gate rather than living in prose, since prose is what just failed: test-api-consistency.R enumerates every exported function and fails on any pair of functions whose shared arguments disagree on relative order, pins where `format` sits relative to the content arguments, and pins data-first. Two pairs are allow-listed with the reason -- a comment box's pixel width and a column's character width share a name but not a concept. Co-Authored-By: Claude Opus 5 <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.
An API review before Phase 6 widens things further. All pre-release, so no user
code breaks.
1.
xl_sheet(): 23 arguments → 15Phase 4's nine view settings went in flat, while
page = xl_page_setup()hadalready established the pattern for exactly this kind of cluster. They now
follow it:
view = xl_sheet_view(...).freezedeliberately stays onxl_sheet()— it is the single most commonworksheet option, and making the common case wordier to tidy the rare ones would
be a poor trade.
split, its rarely-used divider variant, moves; the two staymutually exclusive and the error now names both spellings.
Done now so Phase 6's
validation =lands in a function that isn't alreadyoverloaded.
2. A stale documentation claim
write_xlsx(format_headers)still said the header would be "centered and bold".That stopped being true when the header format became configurable via
xl_properties(header_format = ). It now describes what the argument does andpoints at the setting;
xl_workbook()@inheritParamsit rather than carryingits own paraphrase.
3. One concept, three argument names
xl_hyperlink(name =),xl_hyperlink_cell(value =)andxl_cell_general(value =)all mean the text displayed instead of a URL. Thenames differ only because
xl_hyperlink()mirrors Excel'sHYPERLINK()argument.
nameis released API so it stays; the docs now state theequivalence.
4. Duplicated coercion
if (is.factor(x)) x <- as.character(x); stopifnot(is.character(x))appearedverbatim in all three cell shorthands. Now
.as_character_arg(), which alsoreports the argument name and the class it got instead of
stopifnot()'sexpression dump.
5. Most-probable-first argument order
xl_font()leads withbold, italic, color, size, namerather thanname, size, color, bold, italicxl_comment()leads withauthorbeforeformatEvery call site names its arguments past the first, so nothing positional
depended on the old order — verified by grep before changing them.
Correction to the review
I initially flagged the
formatargument as documented seven times withinconsistent wording. On inspection that is mostly legitimate variation, not
duplication:
xl_comment()uses a fill-and-font subset,xl_rich_run()afont-only subset, and the others a full format. Unifying them would have lost
the distinction. Only
col_names/format_headerswere genuinely the sameargument described twice, and those are now inherited.