Closed clauswilke closed 3 years ago
Do you mean this line is not enough?
I hadn't seen this earlier. Maybe I looked at an outdated code version. In any case, the meaning of NA
should be explained. I would suggest to use the following as a template:
https://github.com/r-lib/pkgload/blob/dece602ffcf99982c0d754ee2e795e9f1d72e514/R/load.r#L65-L67
It has different semantics than compile
argument of load_all()
so the current documentation is correct; it matters only when it's TRUE
.
So, I think we should
compile
to FALSE
, instead of NA
, to avoid confusion@inheritParams pkgload::load_all
or add some implementation to follow the behaviour of load_all()
.
Yes, agreed, if NA
is not a separate option then the default should be FALSE
.
add some implementation to follow the behaviour of
load_all()
.
I don't think this is a very terrible idea, btw. Here's my quick implementation:
https://github.com/extendr/rextendr/compare/main...yutannihilation:poc/handle-na-compile-arg
@Ilia-Kosenkov I think you should have some opinion here. Which do you prefer to fix the document or the implementation?
@yutannihilation,
I did not touch this param because I believe it came from your PR (correct me if I am wrong).
I looked into pkgload::load_all()
docs:
`compile` | If TRUE always recompiles the package; if NA recompiles if needed (as determined by pkgbuild::needs_compile()); if FALSE, never recompiles.
To be honest, I find this behavior reasonable, and applicable to our case as well. register_extendr
is an exported function, it is good to have a flag that controls recompilation. So, we can mimic how pkgload::load_all
behaves, and on NA
invoke ours needs_compilation()
and pkgbuild::needs_compile()
to determine if compilation needed. compile = NA
can be used internally, while compile = TRUE
and compile = FALSE
can be valid user cases, one to enforce recompilation (if library depends on some resource other than Rust/C/Fortarn code, changes to which we do not track), another -- to re-generate wrappers without compiling Rust code.
I am certainly not in favor of removing compile
parameter, turning register_extendr
into a black box god function that does everything under the hood, over which the user has no control.
I would go even further and introduce a flag (and re-introduce the function we deprecated earlier) that determines if wrappers should be re-generated. Currently, every call to rextendr::document()
overwrite wrppers, which is not really needed.
I believe it came from your PR
Yeah, basically this issue is my fault that I didn't consider this parameter carefully.
I am certainly not in favor of removing
compile
parameter, turningregister_extendr
into a black box god function that does everything under the hood, over which the user has no control.
I don't understand this part. I believe none of us here insists on removing the parameter?
Anyway, I think now we agreed to fix the implementation, not the document. I'll create a pull request for this so that we can discuss further.
@yutannihilation, perhaps I misunderstood you, I am sorry. I agree that it is better to fix implementation than the document.
No worries, maybe my explanation was too terse...
Currently this parameter is not documented: https://github.com/extendr/rextendr/blob/74817048d578c640e77df740e1fa00071320a011/R/register_extendr.R#L120-L128