Conversation
…ncy because of a change in how messages were being printed and tested
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new mdate scales and S3 registration have correctness/CRAN-check issues (Suggests-default evaluation and unqualified registerS3method) that should be fixed before release.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prepares the autograph v1.2.3 release by adding {messydates}-backed ggplot2 scales for messy dates, introducing multiple layout/drawing quality diagnostics, and refactoring palette/a11y-related checks, with accompanying docs/tutorial and test updates.
Changes:
- Add
scale_x_mdate()/scale_y_mdate()and registerscale_type.mdate()on load to enable automatic date axes formessydates::mdate. - Add new drawing-quality check functions (
check_crossings(),check_slopes(),check_lengths(),check_angles(),check_drawing()) plus tests and updated tutorial/vignette content. - Refactor colour “checks” into
R/theme_aesthetics.R, update docs/man pages, and adjust tests from message-based to warning-based expectations.
File summaries
| File | Description |
|---|---|
| vignettes/articles/visualising-networks.Rmd | Reorders/expands isolate & layout-quality tutorial content and examples. |
| inst/tutorials/autograph1/visualisation.Rmd | Mirrors tutorial reordering and adds layout-quality exercise content. |
| R/scale_mdate.R | Adds ggplot2 scales for messydates::mdate and a scale_type.mdate() hook. |
| R/zzz.R | Registers the scale_type.mdate S3 method during package load. |
| tests/testthat/test-scale_mdate.R | Adds tests validating mdate scales and rendering (skipped if {messydates} missing). |
| R/layout_aesthetics.R | Introduces drawing-quality check_* functions and check_drawing() summary. |
| tests/testthat/test-layout_aesthetics.R | Adds unit tests for the new drawing check functions and summary output. |
| R/graph_costs.R | Removes legacy layout cost/check implementations (migrated into R/layout_aesthetics.R). |
| tests/testthat/test-functional_layouts.R | Updates deprecation expectations to warnings; adds functional audit for all plot-based check_* functions. |
| tests/testthat/test-grapht.R | Updates deprecated argument behavior expectation from message to warning. |
| tests/testthat/test-functional_coverage.R | Updates diffusion “no spread” behavior expectation from message to warning. |
| R/theme_colorblind.R | Narrows to simulation docs/implementation and points users to palette scoring elsewhere. |
| R/theme_aesthetics.R | Adds check_separation() / check_contrast() palette scoring and documentation. |
| man/mdate_scales.Rd | New generated Rd for mdate scales. |
| man/check_layout.Rd | Updates generated docs to include new drawing checks and expanded guidance. |
| man/check_colors.Rd | New generated Rd for palette scoring functions. |
| man/theme_colorblind.Rd | Updates generated docs to remove moved aliases and add seealso links. |
| man/theme_set.Rd | Adds check_colors to “Other themes” seealso list. |
| man/theme_medium.Rd | Adds check_colors to “Other themes” seealso list. |
| man/list_fonts.Rd | Adds check_colors to “Other themes” seealso list. |
| NAMESPACE | Exports new functions and registers print.check_drawing. |
| DESCRIPTION | Bumps version to 1.2.3; raises {manynet} minimum; adds {messydates} to Suggests. |
| NEWS.md | Adds v1.2.3 release notes matching the PR description. |
| cran-comments.md | Updates CRAN submission notes. |
| .github/CONTRIBUTING.md | Documents {messydates} suggestion rationale and .onLoad() registration approach. |
Review details
Files not reviewed (7)
- man/check_colors.Rd: Generated file
- man/check_layout.Rd: Generated file
- man/list_fonts.Rd: Generated file
- man/mdate_scales.Rd: Generated file
- man/theme_colorblind.Rd: Generated file
- man/theme_medium.Rd: Generated file
- man/theme_set.Rd: Generated file
- Files reviewed: 18/26 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+49
to
+73
| scale_x_mdate <- function(..., FUN = messydates::vmin) { | ||
| .mdate_scale("x", ..., FUN = FUN) | ||
| } | ||
|
|
||
| #' @rdname mdate_scales | ||
| #' @export | ||
| scale_y_mdate <- function(..., FUN = messydates::vmin) { | ||
| .mdate_scale("y", ..., FUN = FUN) | ||
| } | ||
|
|
||
| # Builds a {ggplot2} date scale, then swaps in a transformation that resolves | ||
| # an mdate to a Date first. {ggplot2}'s own date transformation rejects | ||
| # anything that is not already a Date. The scale is otherwise untouched, so | ||
| # its breaks, labels and guides stay those of a date axis. | ||
| .mdate_scale <- function(aesthetic, ..., FUN) { | ||
| thisRequires("messydates") | ||
| scale <- if (aesthetic == "x") ggplot2::scale_x_date(...) else | ||
| ggplot2::scale_y_date(...) | ||
| transformation <- if (is.null(scale$get_transformation)) scale$trans else | ||
| scale$get_transformation() | ||
| transformation$transform <- function(x) { | ||
| if (inherits(x, "mdate")) x <- as.Date(x, FUN = FUN) | ||
| if (inherits(x, "POSIXct")) x <- as.Date(x) | ||
| structure(as.numeric(x), names = names(x)) | ||
| } |
Comment on lines
+79
to
+83
| .onLoad <- function(libname, pkgname) { | ||
| registerS3method("scale_type", "mdate", scale_type.mdate, | ||
| envir = asNamespace("ggplot2")) | ||
| invisible(NULL) | ||
| } |
Comment on lines
+603
to
+607
| **Add two unconnected characters to `fict_lotr` and compare keeping them with noting them in the legend.** | ||
|
|
||
| ```{r isolates, exercise=TRUE, fig.width=9} | ||
| lotr_iso <- to_unlabelled(fict_lotr) |> | ||
| add_nodes(10) |
Comment on lines
+556
to
+560
| **Add two unconnected characters to `fict_lotr` and compare keeping them with noting them in the legend.** | ||
|
|
||
| ```{r isolates, fig.width=9} | ||
| lotr_iso <- to_unlabelled(fict_lotr) |> | ||
| add_nodes(10) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
+ Coverage 90.47% 91.88% +1.41%
==========================================
Files 37 40 +3
Lines 4576 4697 +121
==========================================
+ Hits 4140 4316 +176
+ Misses 436 381 -55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Package
{messydates}toSuggests{manynet}floor to 2.3.1Theming
scale_x_mdate()andscale_y_mdate()(closes Add ggplot2 support formdatecolumns globalgov/messydates#104)FUNresolves each messy date to one date;messydates::vminby defaultR/theme_aesthetics.RLayouts
check_crossings()for how many other ties each tie crossescheck_slopes()for the slope each tie is drawn atcheck_lengths()for the total, max, and variance of tie lengthscheck_angles()for the angular resolution at each nodecheck_drawing(), which reports those checks pluscheck_stress()Tutorials