Skip to content

chore: improve errors in iterators.R #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: improve errors in iterators.R #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 19 additions & 31 deletions R/iterators.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,7 +1079,7 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
}
if (is.logical(ii) && (length(ii) != length(x) && length(ii) != 1)) {
cli::cli_abort(
"Error: Logical index length does not match the number of edges. Recycling is not allowed."
"Logical index length does not match the number of edges. Recycling is not allowed."
)
}

Expand Down Expand Up @@ -1144,14 +1144,11 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @name igraph-vs-attributes
#' @export
`[[<-.igraph.vs` <- function(x, i, value) {
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how value could be missing. I've added tests for when name is missing, but value missing would mean nothing on the right side of the arrow?! Am I missing something obvious?

In any case we need to improve the mysterious "Invalid indexing" error.

Copy link
Contributor

@schochastics schochastics Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With main:

g <- make_ring(10)
V(g)[10][["name"]] <- "z"
#> Error in `[[<-.igraph.vs`(`*tmp*`, "name", value = "z"): invalid indexing
V(g)[["name"]] <- letters[1:10]
#> Error in `[[<-.igraph.vs`(`*tmp*`, "name", value = c("a", "b", "c", "d", : invalid indexing

With stoop2 branch

g <- make_ring(10)
V(g)[10][["name"]] <- "z"
#> Error in `[[<-`:
#> ! Can't find "name" for attribute.
V(g)[["name"]] <- letters[1:10]
#> Error in `[[<-`:
#> ! Can't find "name" for attribute.

Created on 2025-07-23 with reprex v2.1.1

Not sure this helps with your question because this function confuses me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you were able to get this error, no name, but no value makes 0 sense to me. I find the message "invalid indexing" extremely confusing.

) {
cli::cli_abort("Invalid indexing.")
if (!rlang::has_name(attributes(value), "name")) {
cli::cli_abort("Can't find {.val name} for attribute.")
}
if (is.null(get_vs_graph(x))) {
stop("Graph is unknown.")
cli::cli_abort("Can't find graph.")
}
value
}
Expand All @@ -1166,14 +1163,11 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @name igraph-es-attributes
#' @export
`[[<-.igraph.es` <- function(x, i, value) {
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
) {
stop("Invalid indexing.")
if (!rlang::has_name(attributes(value), "name")) {
cli::cli_abort("Can't find {.val name} for attribute.")
}
if (is.null(get_es_graph(x))) {
stop("Graph is unknown.")
cli::cli_abort("Can't find graph.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether we could improve this.

}
value
}
Expand Down Expand Up @@ -1239,7 +1233,7 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
`$.igraph.vs` <- function(x, name) {
graph <- get_vs_graph(x)
if (is.null(graph)) {
cli::cli_abort("Graph is unknown")
cli::cli_abort("Can't find graph.")
}
res <- vertex_attr(graph, name, x)
if (is_single_index(x)) {
Expand Down Expand Up @@ -1292,7 +1286,7 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
`$.igraph.es` <- function(x, name) {
graph <- get_es_graph(x)
if (is.null(graph)) {
cli::cli_abort("Graph is unknown")
cli::cli_abort("Can't find graph.")
}
res <- edge_attr(graph, name, x)
if (is_single_index(x)) {
Expand All @@ -1310,7 +1304,7 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @export
`$<-.igraph.vs` <- function(x, name, value) {
if (is.null(get_vs_graph(x))) {
cli::cli_abort("Graph is unknown")
cli::cli_abort("Can't find graph.")
}
attr(x, "name") <- name
attr(x, "value") <- value
Expand All @@ -1325,7 +1319,7 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @family vertex and edge sequences
`$<-.igraph.es` <- function(x, name, value) {
if (is.null(get_es_graph(x))) {
cli::cli_abort("Graph is unknown")
cli::cli_abort("Can't find graph.")
}
attr(x, "name") <- name
attr(x, "value") <- value
Expand All @@ -1336,11 +1330,8 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @export
`V<-` <- function(x, value) {
ensure_igraph(x)
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
) {
cli::cli_abort("invalid indexing")
if (!rlang::has_name(attributes(value), "name")) {
cli::cli_abort("Can't find {.val name} for vertex attribute.")
}
i_set_vertex_attr(
x,
Expand All @@ -1360,11 +1351,8 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @export
`E<-` <- function(x, path = NULL, P = NULL, directed = NULL, value) {
ensure_igraph(x)
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
) {
cli::cli_abort("invalid indexing")
if (!rlang::has_name(attributes(value), "name")) {
cli::cli_abort("Can't find {.val name} for edge attribute.")
}
i_set_edge_attr(
x,
Expand Down Expand Up @@ -1563,7 +1551,7 @@ as_igraph_vs <- function(graph, v, na.ok = FALSE) {
if (is.character(v) && "name" %in% vertex_attr_names(graph)) {
v <- as.numeric(match(v, V(graph)$name))
if (!na.ok && any(is.na(v))) {
cli::cli_abort("Invalid vertex names")
cli::cli_abort("Invalid vertex names {.arg v}.")
}
v
} else {
Expand All @@ -1575,7 +1563,7 @@ as_igraph_vs <- function(graph, v, na.ok = FALSE) {
res <- as.numeric(v)
}
if (!na.ok && any(is.na(res))) {
cli::cli_abort("Invalid vertex name(s)")
cli::cli_abort("Invalid vertex name(s) {.arg v}.")
}
res
}
Expand Down Expand Up @@ -1618,7 +1606,7 @@ as_igraph_es <- function(graph, e) {
res <- as.numeric(e)
}
if (any(is.na(res))) {
cli::cli_abort("Invalid edge names")
cli::cli_abort("Invalid edge names {.arg e}.")
}
res
}
Expand All @@ -1638,7 +1626,7 @@ parse_op_args <- function(..., what, is_fun, as_fun, check_graph = TRUE) {
args <- list(...)

if (any(!sapply(args, is_fun))) {
cli::cli_abort("Not {what} sequence")
cli::cli_abort("Not {what} sequence.")
}

## get the ids of all graphs
Expand Down
18 changes: 17 additions & 1 deletion tests/testthat/_snaps/iterators.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,21 @@
E(g)[c(TRUE, FALSE)]
Condition
Error in `FUN()`:
! Error: Logical index length does not match the number of edges. Recycling is not allowed.
! Logical index length does not match the number of edges. Recycling is not allowed.

# `[[<-.igraph.es` and `V<-` error well

Code
V(g) <- "blue"
Condition
Error in `V<-`:
! Can't find "name" for vertex attribute.

---

Code
E(g)[1] <- "blue"
Condition
Error in `[<-`:
! Can't find "name" for attribute.

16 changes: 16 additions & 0 deletions tests/testthat/test-iterators.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,3 +447,19 @@ test_that("logical indices are not recycled", {
expect_snapshot(V(g)[c(TRUE, FALSE)], error = TRUE)
expect_snapshot(E(g)[c(TRUE, FALSE)], error = TRUE)
})

test_that("`[[<-.igraph.es` and `V<-` error well", {
g <- make_(
ring(10),
with_vertex_(
name = LETTERS[1:10],
color = sample(1:2, 10, replace = TRUE)
)
)
expect_snapshot(error = TRUE, {
V(g) <- "blue"
})
expect_snapshot(error = TRUE, {
E(g)[1] <- "blue"
})
})
Loading