Skip to content

Strip external generics on build#662

Open
hadley wants to merge 5 commits into
mainfrom
strip-external-generics
Open

Strip external generics on build#662
hadley wants to merge 5 commits into
mainfrom
strip-external-generics

Conversation

@hadley
Copy link
Copy Markdown
Member

@hadley hadley commented May 28, 2026

Fixes #364

@lawremi
Copy link
Copy Markdown
Collaborator

lawremi commented May 29, 2026

Might want to clarify in the docs that it should come after all method registration. The example in zzz.R shows that, but it might help to be explicit.

This also raises the idea of a combined function S7::load() that clears generics and defines .onLoad() via side-effect. Probably not a very good idea, because confusion could result from another .onLoad() being defined. I wonder if the namespace system could support .onLoad.<foo> so that there can be multiple hooks.

@hadley
Add explicit package tests
4b48c69 
And refactor out some package helpers
hadley
hadley commented May 29, 2026
View reviewed changes
Comment thread tests/testthat/t4/R/t4.R
S7::method(t3_s4, t4_class) <- function(x) "s4-dispatch"

.onLoad <- function(libname, pkgname) {
S7::S4_register(t4_class)
Copy link
Copy Markdown
Member Author

@hadley hadley May 29, 2026

Choose a reason for hiding this comment

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

@lawremi Claude discovered this while writing this test. Does that seem correct to you? (i.e. that S4_register(), which calls setOldClass() has to be called onLoad)

Copy link
Copy Markdown
Member Author

@hadley hadley May 29, 2026

Choose a reason for hiding this comment

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

Or maybe this is related to the environment fixes in #643?

Copy link
Copy Markdown
Collaborator

@lawremi lawremi May 30, 2026

Choose a reason for hiding this comment

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

Shouldn't be necessary to call S4_register() at runtime. All it does is call setOldClass() which of course works at build time. Strangely, Gemini suggested the same thing when converting a package to use S7.

Copy link
Copy Markdown
Member Author

@hadley hadley Jun 1, 2026

Choose a reason for hiding this comment

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

Let's hope that #663 fixes this. I'll hold off on merging until that's confirmed.

@hadley
Copy link
Copy Markdown
Member Author

hadley commented May 29, 2026 •
edited
Loading

Hmmm, I wonder if we can call setHook(packageEvent(current_package, "onLoad")) to register an onLoad hook in S7_on_build()?

...

Nope, that doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lawremi lawremi lawremi left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generics are embedded when defining methods

2 participants

@hadley @lawremi