Skip to content
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.

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

Templates without importing constructive #456

Open
krlmlr opened this issue Jul 11, 2024 · 4 comments · May be fixed by #462
Open

Templates without importing constructive #456

krlmlr opened this issue Jul 11, 2024 · 4 comments · May be fixed by #462

Comments

@krlmlr
Copy link
Contributor

krlmlr commented Jul 11, 2024

tidyverse/duckplyr#197

I should have split this into multiple commits, but haven't.

Can we also save the template into a sensible file name by default?

@moodymudskipper
Copy link
Collaborator

What is wrong with importing constructive ? All the functions imported through the template (and all other potentially useful functions for extension) are prefixed with .cstr_ so they're unlikely to collide with a package's function. I could add a "no_import" argument but I'd like to understand the value.

About auto saving I'm not sure, it'd be a permanent side effect by default, not obvious to undo, with a name that's not super explicit about it, I find it a bit rude when functions do that. That might discourage users to play around, + the risk of overriding an existing script, or to block the process if we don't overwrite.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jul 11, 2024

It's not wrong, it's just that constructive is not needed for operating the duckplyr package. It's only useful for me as a developer. I can also add the corresponding files to .Rbuildignore . But I'd argue that constructive should almost always be in "Suggests", for that reason -- you don't need the methods until you use constructive, and then they're there.

No strong opinion regarding autosaving.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jul 11, 2024

I can't easily .Rbuildignore these files, precisely because methods have to be registered.

My approach is to use vctrs::s3_register(), which is available under an "unlicense".

@moodymudskipper
Copy link
Collaborator

Good point about having constructive in suggests, we need a message suggesting use this::use_package("constructive", "Suggests") when using extension functions.

I understand now, I thought we could use the import tags and have constructive in Suggests but indeed checks fail.

rlang::on_load({vctrs::s3_register()}) is 2 more Imports dependencies though, only 1 if we go through the trouble of explaining that s3_register can be copy pasted. I think the no dep version is to have it in .onLoad() with if (requireNamespace("constructive", quietly = TRUE)) s3_register(...).

It's less plug and play unfortunately but I understand the value.
This will require a new pass on the constructive.example package as well 😞

@moodymudskipper moodymudskipper linked a pull request Jul 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants