chore: small polish bundle (typos, dogfood, doc accuracy) - #101
Merged
Conversation
3 tasks
There was a problem hiding this comment.
Pull request overview
This PR bundles a set of small “polish” changes across the package: fixing a couple of minor typos in dock_from_desc(), tightening a brittle regression assertion, and improving documentation/comment accuracy around PPM handling and API usage.
Changes:
- Fix two
length(x > 0)typos indock_from_desc()and correct a non-interpolatedstop()string. - Dogfood the
dock$ARG(name, default = ...)API indock_from_renv(), and clarify PPM-host rewrite behavior in code comments +NEWS.md. - Remove a duplicate roxygen
@exporttag and tighten a brittle test regex to avoid substring-based false positives.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/testthat/test-dock_from_renv.R |
Tightens “no remotes” assertions to check only real install patterns. |
R/dockerignore.R |
Removes a duplicate roxygen @export tag. |
R/dock_from_renv.R |
Uses dock$ARG(..., default=...) and clarifies PPM rewrite behavior in comments. |
R/dock_from_desc.R |
Fixes length(...) > 0 typos and removes non-interpolated {pkgbuild} braces in a stop() message. |
NEWS.md |
Adds a dev NEWS bullet for the polish bundle and clarifies PPM rewrite wording. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,13 @@ | |||
| # dockerfiler (development version) | |||
|
|
|||
| - chore: small polish bundle (no behavioural changes for end users): | |||
| `packagemanager.rstudio.com` and internal mirrors) are preserved on | ||
| rewrite. Multi-entry `repos` vectors and non-PPM repos are left | ||
| are preserved as-is. The user's PPM scheme and host (so a | ||
| `packagemanager.rstudio.com` URL stays on rstudio.com) is preserved |
VincentGuyader
force-pushed
the
chore/polish-bundle
branch
from
May 5, 2026 21:24
0753850 to
ff89a7c
Compare
Cosmetic / hygiene changes surfaced by the post-0.2.6 audit. No
behavioural changes for end users.
- `R/dock_from_desc.R`: `length(packages_on_cran > 0)` was a typo
for `length(packages_on_cran) > 0` (the parens encompassed the
comparison). The buggy form happens to evaluate to the right
truth value because `length(<bool_vec>) == length(<input_vec>)`,
but the intent is now obvious. Same fix for the
`packages_not_on_cran` site.
- `R/dock_from_desc.R`: drop fake-`{glue}` braces from
`stop("please install {pkgbuild}")`; the message is not
interpolated, the literal `{pkgbuild}` was reaching end users.
- `R/dock_from_renv.R`: use the new `dock$ARG("RENV_PATHS_CACHE",
default = renv_paths_cache)` form (added in #91) instead of
inlining the `=` via `sprintf()`. Internal dogfood of the new
API.
- `R/dock_from_renv.R` + `NEWS.md` 0.2.6 bullet: clarify that the
PPM patch only fires for URLs on the official `posit.co` /
`rstudio.com` PPM hosts. The previous wording suggested
internal mirrors were "preserved on rewrite", but they are not
- they fall outside the PPM detection regex and are skipped
entirely. Internal mirrors stay untouched (which is the correct
outcome), just not via the rewrite branch.
- `R/dockerignore.R`: drop a duplicate `@export` tag.
- `tests/testthat/test-dock_from_renv.R`: tighten the regression
assertion `expect_false(any(grepl("remotes", ...)))` to
`grepl("install\\.packages\\([^)]*remotes", ...)` plus a check
for `remotes::install_version`. The previous form was brittle
(would false-positive on any incidental occurrence of the
substring "remotes" elsewhere in the generated Dockerfile, e.g.
in a future host name).
Tests: 150 PASS / 0 FAIL.
`{pkg}` is the convention used in roxygen and user-facing strings
across this codebase; the polish bundle had stripped them by mistake.
Update NEWS to drop the now-obsolete bullet item.
After rebasing on master (which renamed `(development version)` to 0.3.0 with subheadings), git auto-merge placed the polish-bundle bullet under `## New features` because that's where the first top-of-section bullet landed. The bullet's content is mostly typo / dead-code / internal-form fixes, so move it under `## Bug fixes` and drop the now-misleading `chore:` prefix.
"scheme and host ... is preserved" -> "are preserved" (compound subject). Caught by Copilot review at the original HEAD; the correction was deferred and is applied now.
VincentGuyader
force-pushed
the
chore/polish-bundle
branch
from
May 5, 2026 21:34
ff89a7c to
1e78b70
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dock$RUN("R -e 'install.packages(\"remotes\")'") | ||
|
|
||
| if (length(packages_on_cran > 0)) { | ||
| if (length(packages_on_cran) > 0) { |
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.
Cosmetic / hygiene changes from the post-0.2.6 audit. No behavioural
changes for end users. Bundled to avoid 6 micro-PRs for purely
stylistic stuff.
Changes
R/dock_from_desc.Rx2:length(packages_on_cran > 0)wasa typo for
length(packages_on_cran) > 0(the parens encompassedthe comparison). The buggy form happened to evaluate to the right
truthy value because
length(<bool_vec>) == length(<input_vec>),but the intent is now obvious. Same fix on
packages_not_on_cran.R/dock_from_desc.R: drop fake-{glue}braces fromstop("please install {pkgbuild}"). The string was notinterpolated, so end users hitting that error saw the literal
{pkgbuild}text.R/dockerignore.R: duplicate@exporttag removed.R/dock_from_renv.R: dogfood the newdock$ARG(name, default = ...)form (added in feat(ARG): support an optional default value #91) instead of inlining the=viasprintf(). Identical Dockerfile output, just goes through the public API.R/dock_from_renv.R+NEWS.md0.2.6 bullet: clarify that the PPM patch only fires on the officialposit.co/rstudio.comPPM hosts. The previous wording suggested internal mirrors were "preserved on rewrite", but the PPM detection regex doesn't match them at all - they fall through and are simply left untouched (which is the correct outcome, just not via the rewrite branch).tests/testthat/test-dock_from_renv.R: tightenexpect_false(any(grepl("remotes", ...)))togrepl("install\\.packages\\([^)]*remotes", ...)+ a check forremotes::install_version. The previous form was brittle - anyfuture incidental occurrence of the substring "remotes" anywhere
in the generated Dockerfile (e.g. in a host name) would
false-positive.
NOT in this bundle
lifecycle::deprecate_warn()for thedistrodeprecation):requires adding
lifecycletoImports:, deferred for separatedecision.
if (missing(out))branch (#98) #99 (dead-code)and the planned coverage PR.
Test plan
devtools::test()-> 150 PASS / 0 FAIL.byte-identical output. G4 strengthens an existing test rather than
adding one.