Skip to content

refactor: remove lazyeval usage in [.igraph.vs#1445

Merged
aviator-app[bot] merged 4 commits intomainfrom
tschuess-lazyeval
Aug 27, 2024
Merged

refactor: remove lazyeval usage in [.igraph.vs#1445
aviator-app[bot] merged 4 commits intomainfrom
tschuess-lazyeval

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Aug 13, 2024

Fix #1426

I'm less confident about this one.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Aug 13, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@maelle maelle changed the title refactor: remove lazyeval usage from R/iterators.R refactor: remove lazyeval usage in [.igraph.vs Aug 13, 2024
@maelle maelle force-pushed the tschuess-lazyeval branch 4 times, most recently from 3c95477 to 9fa5965 Compare August 14, 2024 07:11
Comment thread R/iterators.R
Comment thread src/cpp11.cpp
extern SEXP R_igraph_write_graph_ncol(SEXP, SEXP, SEXP, SEXP);
extern SEXP R_igraph_write_graph_pajek(SEXP, SEXP);
extern SEXP UUID_gen(SEXP);
extern SEXP make_lazy(SEXP, SEXP, SEXP);
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.

this file is read-only, how was I supposed to remove these lines then? 😅

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regenerate using R -e 'cpp11::cpp_register()'. This is why I keep insisting that these procedures must be documented in wiki pages.

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.

oh right, thanks! or the contributing guide, a more usual place for R packages. 😸

Copy link
Copy Markdown
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Nice!

Comment thread R/iterators.R Outdated
if (length(args) < 1 ||
(length(args) == 1 && inherits(args[[1]]$expr, "name") &&
as.character(args[[1]]$expr) == "")) {
## or empty (what is the test case for this??)
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.

g[,] perhaps? Or g[, na_ok = FALSE] ?

Comment thread R/iterators.R Outdated
as.character(args[[1]]$expr) == "")) {
## or empty (what is the test case for this??)
## try() because of .env$something would otherwise error
first_is_empty <- !nzchar(paste(rlang::quo_get_expr(args[[1]]), collapse = ""))
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.

rlang::is_missing(args[[1]]) ? But we need a test case...

The .ignore_empty argument to rlang::enquos() might take care of that.

Comment thread R/iterators.R
@maelle maelle force-pushed the tschuess-lazyeval branch 2 times, most recently from 3fa4cd0 to 103f459 Compare August 22, 2024 11:56
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Aug 22, 2024

Does this make any sense:

library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union
g <- make_ring(10)
V(g)[,]
#> + 10/10 vertices, from 847d690:
#>  [1]  1  2  3  4  5  6  7  8  9 10
V(g)[, na_ok = FALSE]
#> + 10/10 vertices, from 847d690:
#>  [1]  1  2  3  4  5  6  7  8  9 10
E(g)[,]
#> + 10/10 edges from 847d690:
#>  [1] 1-- 2 2-- 3 3-- 4 4-- 5 5-- 6 6-- 7 7-- 8 8-- 9 9--10 1--10
E(g)[, na_ok = FALSE]
#> + 0/10 edges from 847d690:

Created on 2024-08-22 with reprex v2.1.0

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Aug 22, 2024

with the version of main it's actually worse

library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union
g <- make_ring(10)
V(g)[,]
#> Error in eval(x$expr, data, x$env): argument is missing, with no default
V(g)[, na_ok = FALSE]
#> + 10/10 vertices, from 2584d97:
#>  [1]  1  2  3  4  5  6  7  8  9 10
E(g)[,]
#> Error in eval(x$expr, data, x$env): argument is missing, with no default
E(g)[, na_ok = FALSE]
#> Error in eval(x$expr, data, x$env): argument is missing, with no default

Created on 2024-08-22 with reprex v2.1.0

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Aug 22, 2024

The E(g)[, na_ok = FALSE] case looks odd, can we clean that up as part of this PR? Looks good otherwise.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Aug 27, 2024

Actually [.igraph.es has no na_ok argument! 🤦‍♀️

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Aug 27, 2024

This pull request can't be queued because it's currently a draft.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Aug 27, 2024

This pull request can't be queued because it's currently a draft.

@maelle maelle marked this pull request as ready for review August 27, 2024 12:45
@maelle maelle force-pushed the tschuess-lazyeval branch from 76626c2 to 22cd596 Compare August 27, 2024 12:45
@aviator-app aviator-app bot force-pushed the tschuess-lazyeval branch from 22cd596 to 5a7dde9 Compare August 27, 2024 13:42
@aviator-app aviator-app bot force-pushed the tschuess-lazyeval branch from 5a7dde9 to 13151a1 Compare August 27, 2024 14:01
@aviator-app aviator-app bot merged commit 093f35c into main Aug 27, 2024
@aviator-app aviator-app bot deleted the tschuess-lazyeval branch August 27, 2024 15:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

replace vendored lazyeval with rlang

3 participants