Skip to content

fix: preserve pinned versions in DESCRIPTION and accept several dirs in dir.r (#139, #140) - #141

Open
VincentGuyader wants to merge 1 commit into
mainfrom
fix-139-140-version-pins-multi-dir
Open

fix: preserve pinned versions in DESCRIPTION and accept several dirs in dir.r (#139, #140)#141
VincentGuyader wants to merge 1 commit into
mainfrom
fix-139-140-version-pins-multi-dir

Conversation

@VincentGuyader

Copy link
Copy Markdown
Member

Fixes two ways a version constraint written by hand in DESCRIPTION could vanish on att_amend_desc().

#140 - a hand-set version must never silently disappear

When a package was listed under two types (a pinned Imports and a bare Suggests, for instance), att_to_desc_from_is() rebuilt the dependency table by joining the scan result with deps_orig on package only. That multiplied the rows, and the later !duplicated() kept whichever row came first, so the surviving version depended on the order of the rows in DESCRIPTION. A pinned constraint could be reset to * with no message.

The fix collapses deps_orig to one row per package before the join, ordering so that an explicit constraint beats * and, on a tie, Imports beats Suggests. The hand-set version is now kept, deterministically, and when both an Imports and a Suggests row carry a version the Imports one wins. Type resolution itself (a package used from both an Imports and a Suggests location lands in Imports) was already handled by the scan and is unchanged.

#139 - dir.r now accepts several directories

att_amend_desc(dir.r = c("R", "inst")) raised the condition has length > 1: dir.r was filtered as a vector but then tested as a scalar with if (dir.r != ""). Changed to if (!identical(dir.r, "")) at both sites. The extractors (att_from_rscripts, att_from_examples) already vectorised over multiple directories. This lets you declare dependencies used only outside the default scan, such as a deployment entry point under inst/, so their pins are then preserved by the #140 fix.

Tests

New test-att_amend_desc_version_pins.R, written red-first:

  • pin preserved when the package also sits under another type, including when the scan moves it to Suggests;
  • the Imports-side version wins when both types carry different explicit constraints;
  • an Imports/Suggests conflict resolves to Imports with its version;
  • dir.r = c("R", "inst") no longer errors and a dependency used only in inst/ is detected.

Full suite green (77 tests). R CMD check --as-cran on the built tarball: 0 error, 0 warning, 0 relevant note. Submitted to win-builder R-devel.

Closes #139
Closes #140

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request improves {attachment}’s att_amend_desc() dependency rewriting so that maintainer-authored version constraints in DESCRIPTION are preserved deterministically, and extends dir.r to accept multiple directories so additional R-script locations (e.g. inst/) can be scanned.

Changes:

  • Preserve hand-set version pins when a package appears multiple times in DESCRIPTION under different dependency types by collapsing original version rows before joining.
  • Allow dir.r to be a character vector of directories (and document the behavior).
  • Add focused regression tests for version pin preservation and multi-dir dir.r, and update NEWS/man pages.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/testthat/test-att_amend_desc_version_pins.R Adds regression tests covering pinned-version preservation and multi-directory dir.r.
R/att_to_description.R Fixes duplicate-package version-pin loss during DESCRIPTION rewrite; updates dir.r handling and roxygen docs.
NEWS.md Documents the dir.r multi-directory support and the version-pin preservation fix.
man/attachment-deprecated.Rd Updates generated documentation to reflect dir.r now accepting multiple directories.
man/att_amend_desc.Rd Updates generated documentation to reflect dir.r now accepting multiple directories.
Files not reviewed (2)
  • man/att_amend_desc.Rd: Generated file
  • man/attachment-deprecated.Rd: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

dummypackage <- copy_dummy()
on.exit(unlink(dirname(dummypackage), recursive = TRUE), add = TRUE)

# `glue` is detected as a Suggest in dummypackage. Pin it in Imports by hand so

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b3fcf9a. Also corrected two further occurrences of the same singular slip in this file (the test-name and the fakepkg comment), so Imports/Suggests are now written with their canonical plural field names throughout.

@VincentGuyader
VincentGuyader force-pushed the fix-139-140-version-pins-multi-dir branch 2 times, most recently from b3fcf9a to 6e6c43a Compare August 19, 2026 08:54
… in dir.r

Keep a version constraint set by hand in DESCRIPTION when the same package
is listed under two types (a pinned Imports and a bare Suggests, say). The
version is no longer dropped, and an Imports/Suggests conflict resolves to
Imports independently of the row order in DESCRIPTION (#140).

Let dir.r take several directories, e.g. dir.r = c("R", "inst"), so
dependencies used only outside the default scan can be declared. Passing a
vector previously raised "the condition has length > 1" (#139).
@VincentGuyader
VincentGuyader force-pushed the fix-139-140-version-pins-multi-dir branch from 6e6c43a to 66da51a Compare August 19, 2026 09:00
@VincentGuyader
VincentGuyader requested a lite review from Copilot August 19, 2026 11:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.

Files not reviewed (2)
  • man/att_amend_desc.Rd: Generated file
  • man/attachment-deprecated.Rd: Generated file
Suppressed comments (1)

R/att_to_description.R:379

  • The tie-break comment says to prefer Imports over Suggests, but order(..., deps_orig$type) uses lexicographic ordering across all dependency types present in deps_orig (e.g., Depends/Enhances/LinkingTo), which can unintentionally take precedence over Imports/Suggests when the same package appears under other types with a different explicit constraint. Consider implementing an explicit type priority (e.g., Imports first, then Suggests, then everything else) and also treating NA versions consistently (since deps_orig$version == \"*\" can yield NA, affecting ordering).
  # Collapse the original versions to one row per package before the join.
  # A package listed under two types (a pinned Imports and a bare Suggests, say)
  # otherwise multiplies rows in the merge, and the later de-duplication would
  # keep whichever row happened to come first. Order so that an explicit
  # constraint beats "*", and Imports beats Suggests on a tie, so a version set
  # by hand in DESCRIPTION is never silently dropped.
  orig_versions <- deps_orig[
    order(deps_orig$version == "*", deps_orig$type),
    c("package", "version")]
  orig_versions <- orig_versions[!duplicated(orig_versions$package), ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants