type_abline()-alike now respect flip = TRUE - #735
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate validation and mixed-slope transformation issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes type_abline(), type_hline(), and type_vline() respect flipped plots through shared rendering logic.
Changes:
- Adds flip-aware line orientation and slope transformations.
- Consolidates straight-line implementations.
- Updates examples, documentation, NEWS, and snapshot coverage.
Review findings:
R/type_abline.R— moderate (1 vote):NULLparameters can bypass validation; mixed zero/nonzero slopes are transformed incorrectly.R/type_abline.R— nit (1 vote): Add flipped zero-slope and grouped/faceted coverage.R/type_abline.R— nit (1 vote): Add active unflippedtype_abline()coverage, includingb = 0.vignettes/gallery_figs/likert.R— nit (2 votes): Update the remaining flipped Likert examples inR/type_barplot.Rand generated documentation.
File summaries
| File | Summary |
|---|---|
vignettes/gallery_figs/likert.R |
Updates flipped Likert examples. |
R/type_vline.R |
Delegates vertical-line rendering to shared logic. |
R/type_hline.R |
Delegates horizontal-line rendering to shared logic. |
R/type_errorbar.R |
Updates a flipped example. |
R/type_abline.R |
Consolidates line rendering and flip transformations. |
NEWS.md |
Documents the behavior change. |
man/type_errorbar.Rd |
Updates generated errorbar documentation. |
man/type_abline.Rd |
Updates generated line documentation. |
inst/tinytest/test-type_pointrange.R |
Updates a flipped layer example. |
inst/tinytest/test-flip.R |
Adds flipped-line snapshot coverage. |
inst/tinytest/_tinysnapshot/flip_ablines.svg |
Stores expected snapshot output. |
Review details
Files not reviewed (2)
- man/type_abline.Rd: Generated file
- man/type_errorbar.Rd: Generated file
Suppressed comments (4)
R/type_abline.R:117
- Filtering
NULLbefore validation drops the active argument for calls such astype_hline(h = NULL)andtype_vline(v = NULL). These constructors previously calledassert_numeric()on their supplied parameter, so this is a validation regression that can defer the failure to an emptyabline()call. Buildparamsfrom the selected line type and validate those entries before removing unused fields.
params = params[!vapply(params, is.null, logical(1))]
R/type_abline.R:188
- With a numeric continuous
by, the parameter vectors remain unsliced because the!by_continuousbranches are skipped, sobcan contain both zero and nonzero slopes. Theall()check only handles the all-zero case; mixed vectors turn the zero-slope entries intoa = -a / 0andb = 1 / 0rather than drawing those entries as verticalv = alines. Split zero-slope entries and transform only the nonzero slopes, preserving the matching aesthetics for each line.
abline = if (all(params[["b"]] == 0)) {
list(v = params[["a"]])
} else {
list(a = -params[["a"]] / params[["b"]], b = 1 / params[["b"]])
R/type_abline.R:185
- The new flipped snapshot covers scalar
h/vvalues and a nonzero-slope abline, but not thisb == 0branch. That branch is the special-case transformation for a horizontal original line and can regress independently; please add a flipped zero-slope abline test (and, ideally, a grouped/faceted parameter-vector case) so the new shared implementation's edge paths are protected.
abline = if (all(params[["b"]] == 0)) {
R/type_abline.R:193
- The active tests still do not render an unflipped
type_abline():inst/tinytest/test-type_abline.Ronly checks its no-data error, while the new snapshot exercises only the flipped path. Since this refactor replaces the entirea/bdrawing implementation, add an active unflipped case (including theb = 0conversion branch) to catch regressions in the common path.
do.call(abline, c(params, list(col = icol, lty = ilty, lwd = ilwd)))
- Files reviewed: 8/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #733
I ended up consolidating much of the
type_ab/h/v/line()code in the process. I also had to update some ancillary examples that were using the (wrong) workaround for adding straight lines to flipped plots. This may end up triggering revdep issues for other packages, so we'll need to check carefully.MWE
Using the same motivating examples as #733...
Created on 2026-09-16 with reprex v2.1.1