extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
190 stars 28 forks source link

feat!: allow overwriting rextendr templates by `use_extendr` #292

Closed eitsupi closed 1 year ago

eitsupi commented 1 year ago

Resolve #275

This PR allows the use_extendr function to overwrite existing templates. If it is not an interactive session, skip without overwriting.

eitsupi commented 1 year ago

@JosiahParry Could you take a look at this?

eitsupi commented 1 year ago

Please also update NEWS.md since it is a significant change of the behavior.

I think I have already done that.

Ilia-Kosenkov commented 1 year ago

I think I have already done that.

Sorry, sometimes I am just blind to the things I see in front of me :)

eitsupi commented 1 year ago

The behavior when a file already exists has been completely changed.

If it is not an interactive session, nothing is done with the existing file and no error is raised. In an interactive session, we will only be prompted if there is an update, and we can decide whether to overwrite the file on a file-by-file basis.

JosiahParry commented 1 year ago

I can give this a review tomorrow!

JosiahParry commented 1 year ago

This is the behavior I received with your new branch! I quite like it. At first I hesitated with not having an overwrite argument, but it would be really terrible to not realize your lib.rs got overwritten as well.

image

I would, personally, like for there to be a use_makevars() function or similar so that those files can be updated. I will make a new issue for it.

eitsupi commented 1 year ago

Thanks for reviewing this.

Yeah, overwriting lib.rs and Cargo.toml is not ideal. I considered skipping these by default, but I think we should keep the option given the package renaming.

I would, personally, like for there to be a use_makevars() function or similar so that those files can be updated. I will make a new issue for it.

Creating a separate function may be excessive; how about adding an option to use_extendr so that the behavior can be changed to use only those files?

JosiahParry commented 1 year ago

...how about adding an option to use_extendr so that the behavior can be changed to use only those files?

My hesitancy here is that once you start adding too many option to a function it becomes a bit like a magical cauldron that needs special incantations to work how you want it--i.e. the purpose becomes less straight forwrd.

Alternatively, rextendr:::use_rextendr_template() could be made part of the public API so that template files are easier to use.

JosiahParry commented 1 year ago

Once the typo in the doc is fixed and regenerated in the man page I think its good to go!

JosiahParry commented 1 year ago

I'm not able to actually approve so I'm putting up the bat signal @Ilia-Kosenkov

CGMossa commented 1 year ago

I'll review it then; @JosiahParry you should be able to approve.. We'll investigate that too...

eitsupi commented 1 year ago

Approval itself can be done by anyone (There is a repository option to disallow it), but the green check is only given to approvals by those who have the permission to merge, I think.

JosiahParry commented 1 year ago

Upon further review, why is the overwrite argument not provided? Rather it is inferred by interactive(). Could we, instead, have overwrite = NULL as the default argument so that it asks you by default. Then users can provide the option if they so desire

Ilia-Kosenkov commented 1 year ago

If there are still comments, I will merge as soon as they are resolved.

eitsupi commented 1 year ago

Upon further review, why is the overwrite argument not provided? Rather it is inferred by interactive(). Could we, instead, have overwrite = NULL as the default argument so that it asks you by default. Then users can provide the option if they so desire

Indeed, it is annoying to be asked several times if we want to overwrite. I have updated it.

JosiahParry commented 1 year ago

@Ilia-Kosenkov all resolved

... image