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

Merged
krlmlr merged 12 commits into from
Sep 10, 2026
+312 −31
Merged

chore: improve errors in iterators.R #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
971a8c8
chore: improve errors in iterators.R
maelle Jul 22, 2025
2fd131c
Merge origin/main into stoop2
claude Jul 27, 2026
2be846a
tests thx Claude
maelle Aug 12, 2026
59eec13
expand attribute removal
maelle Aug 26, 2026
588cec2
better
maelle Aug 26, 2026
a4e0bc8
name name yay
maelle Aug 26, 2026
b367288
better errors
maelle Aug 26, 2026
a988499
better wording
maelle Aug 26, 2026
85883bc
ok
maelle Aug 26, 2026
f46e70f
more specific
maelle Aug 26, 2026
f8932f7
Merge branch 'main' into stoop2
krlmlr Sep 3, 2026
35ad09f
change to `isTRUE()`
maelle Sep 8, 2026
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
92 changes: 62 additions & 30 deletions R/iterators.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ graph_id.igraph.es <- function(x, ...) {
}

is_complete_iterator <- function(x) {
identical(attr(x, "is_all"), TRUE)
isTRUE(attr(x, "is_all"))
}

set_complete_iterator <- function(x, value = TRUE) {
Expand Down Expand Up @@ -1162,7 +1162,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 @@ -1227,11 +1227,19 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @name igraph-vs-attributes
#' @export
`[[<-.igraph.vs` <- function(x, i, value) {
if (!rlang::has_name(attributes(value), "attr_name")) {

@maelle maelle Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I asked Claude to rename name and value to attr_name and attr_value in this context because I think the double "value" added to my confusion when I first looked at this code.

cli::cli_abort("Can't find {.val name} for vertex attribute.")
}
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
Comment thread
maelle marked this conversation as resolved.
!rlang::has_name(attributes(value), "attr_value") &&
!is_complete_iterator(value)

@krlmlr krlmlr Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need is_complete_iterator(), or can we use a faster and stricter check?

@maelle maelle Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The definition of is_complete_iterator() is

is_complete_iterator <- function(x) {
  identical(attr(x, "is_all"), TRUE)
}

) {
cli::cli_abort("Invalid indexing.")
cli::cli_abort(
c(
"Can't find {.val value} for vertex attribute {.val {attr(value, 'attr_name')}}.",
i = "Removing an attribute is only supported for the whole vertex sequence, e.g. {.code V(g)${attr(value, 'attr_name')} <- NULL}, not a subset. Use {.fn delete_vertex_attr}."
)
)
}
if (is.null(get_vs_graph(x))) {
cli::cli_abort("Graph is unknown.", .internal = TRUE)
Expand All @@ -1249,11 +1257,19 @@ simple_es_index <- function(x, i, na_ok = FALSE) {
#' @name igraph-es-attributes
#' @export
`[[<-.igraph.es` <- function(x, i, value) {
if (!rlang::has_name(attributes(value), "attr_name")) {
cli::cli_abort("Can't find {.val name} for edge attribute.")
}
if (
!"name" %in% names(attributes(value)) ||
!"value" %in% names(attributes(value))
!rlang::has_name(attributes(value), "attr_value") &&
!is_complete_iterator(value)
) {
cli::cli_abort("Invalid indexing.")
cli::cli_abort(
c(
"Can't find {.val value} for edge attribute {.val {attr(value, 'attr_name')}}.",
i = "Removing an attribute is only supported for the whole edge sequence, e.g. {.code E(g)${attr(value, 'attr_name')} <- NULL}, not a subset. Use {.fn delete_edge_attr}."
)
)
}
if (is.null(get_es_graph(x))) {
cli::cli_abort("Graph is unknown.", .internal = TRUE)
Expand Down Expand Up @@ -1322,7 +1338,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 @@ -1375,7 +1391,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 @@ -1393,10 +1409,10 @@ 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
attr(x, "attr_name") <- name
attr(x, "attr_value") <- value
x
}

Expand All @@ -1408,28 +1424,36 @@ 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
attr(x, "attr_name") <- name
attr(x, "attr_value") <- value
x
}

#' @name igraph-vs-attributes
#' @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), "attr_name")) {
cli::cli_abort("Can't find {.val name} for vertex attribute.")
}
if (!rlang::has_name(attributes(value), "attr_value")) {
if (is_complete_iterator(value)) {
return(delete_vertex_attr(x, attr(value, "attr_name")))
}
cli::cli_abort(
c(
"Can't find {.val value} for vertex attribute {.val {attr(value, 'attr_name')}}.",
i = "Removing an attribute is only supported for the whole vertex sequence, e.g. {.code V(g)${attr(value, 'attr_name')} <- NULL}, not a subset. Use {.fn delete_vertex_attr}."
)
)
}
i_set_vertex_attr(
x,
attr(value, "name"),
name = attr(value, "attr_name"),
index = value,
value = attr(value, "value"),
value = attr(value, "attr_value"),
check = FALSE
)
}
Expand All @@ -1443,17 +1467,25 @@ 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), "attr_name")) {
cli::cli_abort("Can't find {.val name} for edge attribute.")
}
if (!rlang::has_name(attributes(value), "attr_value")) {
if (is_complete_iterator(value)) {
return(delete_edge_attr(x, attr(value, "attr_name")))
}
cli::cli_abort(
c(
"Can't find {.val value} for edge attribute {.val {attr(value, 'attr_name')}}.",
i = "Removing an attribute is only supported for the whole edge sequence, e.g. {.code E(g)${attr(value, 'attr_name')} <- NULL}, not a subset. Use {.fn delete_edge_attr}."
)
)
}
i_set_edge_attr(
x,
attr(value, "name"),
name = attr(value, "attr_name"),
index = value,
value = attr(value, "value"),
value = attr(value, "attr_value"),
check = FALSE
)
}
Expand Down
126 changes: 125 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,129 @@
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.

# assigning `NULL` to a subset of vertices/edges errors instead of silently doing nothing

Code
V(g)[1:3]$color <- NULL
Condition
Error in `[<-`:
! Can't find "value" for vertex attribute "color".
i Removing an attribute is only supported for the whole vertex sequence, e.g. `V(g)$color <- NULL`, not a subset. Use `delete_vertex_attr()`.

---

Code
E(g)[1:3]$weight <- NULL
Condition
Error in `[<-`:
! Can't find "value" for edge attribute "weight".
i Removing an attribute is only supported for the whole edge sequence, e.g. `E(g)$weight <- NULL`, not a subset. Use `delete_edge_attr()`.

# assigning `NULL` for a non-existent attribute errors like `delete_vertex_attr()`/`delete_edge_attr()`

Code
V(g)$color <- NULL

@krlmlr krlmlr Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Allowing this here (but not for delete_vertex_...() would be more idiomatic.

Condition
Error in `delete_vertex_attr()`:
! No vertex attribute `color` found.

---

Code
E(g)$weight <- NULL
Condition
Error in `delete_edge_attr()`:
! No edge attribute `weight` found.

# direct misuse of `V<-`/`E<-`/`[<-`/`[[<-` errors well

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

@krlmlr krlmlr Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Improve error message here and below. Can we check if RHS is an iterator object?


---

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

---

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

---

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

---

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

---

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

# querying or setting attributes errors when the graph is unknown

Code
vs$color
Condition
Error in `vs$color`:
! Can't find graph.

---

Code
vs$color <- "blue"
Condition
Error in `$<-`:
! Can't find graph.

---

Code
es$weight
Condition
Error in `es$weight`:
! Can't find graph.

---

Code
es$weight <- 0
Condition
Error in `$<-`:
! Can't find graph.

# `[<-.igraph.vs` reports an internal error when the graph is unknown

@krlmlr krlmlr Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this an internal error?


Code
`[<-.igraph.vs`(vs, 1, value = payload)
Condition
Error in `[<-.igraph.vs`:
! Graph is unknown.
i This is an internal error that was detected in the igraph package.
Please report it at <https://github.com/igraph/rigraph/issues> with a reprex (<https://tidyverse.org/help/>) and the full backtrace.

Loading
Loading