Skip to content

Rename S7_class attribute to _S7_class#689

Open
hadley wants to merge 2 commits into
mainfrom
s7-class-attribute
Open

Rename S7_class attribute to _S7_class#689
hadley wants to merge 2 commits into
mainfrom
s7-class-attribute

Conversation

@hadley

@hadley hadley commented Jun 8, 2026

Copy link
Copy Markdown
Member

This keeps the name as similar as possible, but makes it clear that it's in the namespace S7 reserves for its own usage. This will invalidate S7 objects created with a previous version of S7; let me know if you think I should deal with those automatically.

Closes #677

This keeps the name as similar as possible, but makes it clear that it's in the namespace S7 reserves for its own usage. This will invalidate S7 objects created with a previous version of S7; let me know if you think I should deal with those automatically.

Closes #677
@hadley hadley requested review from lawremi and t-kalinowski June 8, 2026 19:21

@t-kalinowski t-kalinowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do think it's worth the effort for us to allow packages built with the current S7 release to work with the next S7 release, for at least one S7 release.

@hadley hadley requested a review from t-kalinowski June 9, 2026 22:28
@lawremi

lawremi commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

As you've mentioned, the main source of friction with this type of change is that it will break all serialized instances of S7 objects, and it's worth providing an update path.

More generally, Bioconductor has the updateObject() generic for helping with such transitions. I couldn't find much documentation of how updateObject() is used in practice, but basically the class author adds updateObject(x, verbose=TRUE) to accessor methods, where verbose=TRUE outputs a message() reminding the user to call updateObject() to update any serialized instances, while keeping the object valid. Validity methods could also check for obsolete representations and emit a similar error message. Perhaps the tidyverse has its own approach to this. It would be helpful for S7 itself to centrally define an analogous protocol.

@lawremi lawremi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should rely on a general object update protocol (developed as a separate feature, see comment) that directs the user to update any serialized objects. Otherwise, we will always have to support @S7_class, which sort of defeats the purpose of this change.

@hadley

hadley commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Hmmmm, I was thinking that this was a one off breaking change that we'd make as part of the pre 1.0.0 phase of S7, and since we know that there's relatively limited S7 use we'd eventually drop the code. Since there's relatively little code, the cost of maintaining it is low, and IMO it's worth it because it brings some extra consistency. In principle, I don't think we need to support user @S7_class access because it's an internal slot, which is now clear from the docs.

But it's possible that this problem will come up again in the future, and we should prepare for that. I think the way to do that is to add an interface version to S7 objects so that in the future we could either emit a warning about the serialized object being out of date, or automatically upgrade it using the approach you suggest. That said, I don't think we currently have any hook in base R to call such a function so we'd also need to do some exploration in R itself.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants