Skip to content

refactor!: kill igraph.arpack.default and arpack_defaults lists, add arpack_defaults() as a function#762

Closed
maelle wants to merge 6 commits intoigraph:mainfrom
maelle:kill-lists
Closed

refactor!: kill igraph.arpack.default and arpack_defaults lists, add arpack_defaults() as a function#762
maelle wants to merge 6 commits intoigraph:mainfrom
maelle:kill-lists

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Apr 3, 2023

Fix #714

@krlmlr actually I see usage of the old name in reverse dependencies, should I

  • cancel the transformation of arpack_defaults to a function
  • or make PRs to those packages

Whilst replacing occurrences of the old name, I noticed the opportunity to use modifyList(), so this PR does that too but in a different commit. 😇

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Apr 5, 2023

We could allow a function in the places where arpack_default is used?

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Apr 11, 2023

We could allow a function in the places where arpack_default is used?

Could you please elaborate?

In my latest commit I added a maybe too complex way to deprecate the usage of arpack_default for the options argument

  • the argument is evaluated;
  • if it's called "arpack_default" and is a function (it is a function from igraph, and remains a function unless the user creates an object named so), we replace it with arpack_defaults() after showing a deprecation message.

If that sounds like a good idea I'll add it (as an internal function) to other affected calls.

@maelle maelle requested a review from krlmlr April 17, 2023 10:05
Comment thread R/centrality.R Outdated
"1.5.0",
I("arpack_defaults"),
"arpack_defaults()",
details = "So a function, not an object."
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.

I don't understand this wording.

Comment thread R/centrality.R
arpack <- function(func, extra = NULL, sym = FALSE, options = arpack_defaults(),
env = parent.frame(), complex = !sym) {

eval_try <- rlang::eval_tidy(options)
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.

What do we need eval_tidy for? Can't we just force, and if the user has passed a function, we call it?

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.

We only need to catch the case where the argument is arpack_defaults and a function. I'm using rlang::call_args(rlang::current_call())[["options"]] on the following line hence the rlang here, especially as rlang is a dependency.

Now, maybe this whole "hack" is a bad idea.

@maelle maelle requested a review from krlmlr May 5, 2023 06:53
@krlmlr krlmlr marked this pull request as ready for review May 16, 2023 07:17
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.

Looks good! Can you please merge when the checks pass?

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 16, 2023

actually no, I need to expand the hack to all functions here.

@maelle maelle marked this pull request as draft May 16, 2023 08:39
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 16, 2023

actually with ensure_igraph() on the main branch, I'll just close and start again.

@maelle maelle closed this May 16, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 16, 2024
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.

special case the lifecycle transformation of igraph.arpack.default

2 participants