Roxygen documentation suggestions

[github-actions]

Automated roxygen2 documentation suggestions. Review and merge to include them.

Summary: 5 changes applied · 5 not applied · 2 possible bugs

Applied changes (5)

R/ds.acmPlot.R (5)

• description — inaccurate: The description attributes the call to a function named 'acmPlotDS' retrieving 'prediction data', which is correct, but it omits that the actual plot rendering happens client-side with ggplot2 after fetching data, which the original text implies is a simple visualization step rather than a two-stage process.
• details — inaccurate: The details claimed the function takes a 'prediction object' directly, but pred_obj is actually a character string naming an object on the server, and the function also depends on the rms package and server function acmPlotDS, which were not mentioned.
• param:x_breaks — inaccurate: Original said 'optional' without explaining that when omitted, breaks are computed automatically as 5 equally spaced points between the min and max of the first study column.
• param:event_n — inaccurate: Original just said 'number of events (optional)' without describing its effect of appending '(n = event_n)' to the plot title.
• return — inaccurate: The return is a named list of ggplot objects, one per study/server connection, not merely 'a list of ggplot objects from each study' without specifying naming or structure, and there is no disclosure control since this is a client-rendered plot object built from aggregated summary data.

No changes applied (5)

Suggestions whose reason isn't on the accepted list (accept_reasons in config/claude.yml). Adopt one by hand if it's worth it.

style (3)

R/ds.acmPlot.R

• title: Shortened to a plain phrase under 10 words naming the function's action.

  Plot hazard ratios from a DataSHIELD prediction object

• param:line_size: Reworded slightly for consistency and clarity on numeric type and default.

  Numeric value giving the line width of the main hazard ratio line, passed to ggplot2::geom_line (default 2).

• examples: Simplified and clarified the example to better reflect realistic DataSHIELD session setup.

  Not run:

  Assumes an active DataSHIELD session with connections 'conns'

  and a prediction object already created on the server via ds.Predict()

  plots <- ds.acmPlot(pred_obj = "pred_obj",
  outcome_name = "ACM",
  line_color = "darkblue",
  x_label = "BMI",
  event_n = 1000,
  datasources = conns)

  plots[[1]]

clarity (2)

R/ds.acmPlot.R

• param:pred_obj: Clarified that this is a mandatory character string naming a server-side object (e.g., created by ds.Predict), and that a NULL value triggers an error.

  Character string giving the name of a prediction object on the server, typically created with ds.Predict(); required, the function stops if left NULL.

• param:datasources: Clarified default behavior when NULL (uses datashield.connections_find()), which the original text omitted.

  A list of DSConnection-class objects as returned by DSI::datashield.connections_find(); if NULL, the function attempts to find them automatically.

Possible bugs in the code (2)

Noticed while reviewing the documentation — nothing in the code was changed. Please check.

R/ds.acmPlot.R (2)

• x_breaks computed inside lapply is reused across studies after the first iteration overwrites the argument. Inside the lapply loop, if x_breaks is NULL, it is computed from study_data and then reassigned to the variable x_breaks in the enclosing scope on first use; because lapply's closure captures the same x_breaks binding, once it is set for the first study it is no longer NULL for subsequent studies, so later studies reuse the first study's breaks instead of computing their own.
• require(rms) is called inside the loop without checking success or declaring the dependency. require(rms) silently returns FALSE and continues if the rms package is not installed, potentially causing a less informative downstream error instead of a clear dependency message; library(rms) or a check with requireNamespace would be more robust.

---

Update for ed1fbb9

Summary: 4 changes applied · 4 not applied · 2 possible bugs

Applied changes (4)

R/ds.acmPlot.R (4)

• description — inaccurate: The description says the function gets data from 'acmPlotDS' and builds the plot, which is correct, but it should clarify plots are built locally in R from server-aggregated prediction data, not rendered server-side.
• details — inaccurate: The text claims pred_obj is 'a prediction object created by ds.Predict', but the code treats pred_obj only as a character string naming a server-side object and never calls or references ds.Predict; it also omits that the actual server call is to acmPlotDS and that rms must be available.
• param:x_breaks — incomplete: Original did not mention that if left NULL the function computes breaks automatically from the study data range via seq().
• return — inaccurate: The function returns a list of ggplot objects built locally per study from server-aggregated data, not objects computed and returned by the server verbatim, and there is no disclosure control performed in this function itself.

No changes applied (4)

Suggestions whose reason isn't on the accepted list (accept_reasons in config/claude.yml). Adopt one by hand if it's worth it.

style (3)

R/ds.acmPlot.R

• title: Shortened to a plain phrase under 10 words rather than a sentence fragment echoing the role description.

  Plot hazard ratios from a DataSHIELD prediction object

• param:outcome_name: Kept accurate but reworded slightly for consistency with other param docs.

  Character string used as the plot title prefix, e.g. "ACM" or "CVD" (default: "ACM").

• examples: Simplified and clarified the example while keeping it inside dontrun since it requires live DataSHIELD connections and a server-side object.

  \dontrun{

  After setting up DataSHIELD connections and creating a prediction

  object named "pred_obj" on the server using ds.Predict()

  plots <- ds.acmPlot(pred_obj = "pred_obj",
  outcome_name = "ACM",
  line_color = "darkblue",
  x_label = "BMI",
  event_n = 1000)
  plots[[1]]
  }

clarity (1)

R/ds.acmPlot.R

• param:pred_obj: Clarified that it is the name of a server-side object (not a local prediction object) used to build the aggregate call.

  Character string giving the name of a prediction object that already exists on the server (typically created with ds.Predict()); must not be NULL.

Possible bugs in the code (2)

Noticed while reviewing the documentation — nothing in the code was changed. Please check.

R/ds.acmPlot.R (2)

• x_breaks is reassigned inside lapply using a variable from the enclosing scope, which can cause breaks computed for one study to leak into subsequent studies. Because x_breaks is a function argument captured by the closure, if it is NULL on the first call it gets overwritten inside the lapply loop (x_breaks <- seq(...)); since R's lexical scoping with <- inside lapply's anonymous function creates a local binding each iteration, this specific defect does not actually leak across iterations, but relying on this subtlety makes the code fragile and worth flagging for clarity/maintenance.
• require(rms) is called inside the plotting loop without checking success or placing it at the top of the function. require(rms) inside lapply silently returns FALSE and only emits a warning if the rms package is not installed, potentially allowing the function to proceed and fail later with a less clear error rather than stopping immediately with a clear message.