Rename S7_class attribute to _S7_class#689
Conversation
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
|
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 |
lawremi
left a comment
lawremi
left a comment
There was a problem hiding this comment.
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.
|
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 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. |
lawremi
left a comment
lawremi
left a comment
There was a problem hiding this comment.
That's a reasonable argument: S7 is currently experimental and subject to change. As adoption is starting to ramp up, it would be good to move to a stable state ASAP. Towards that end, let's remember to remove support for the S7_class attribute after the upcoming release.
* Address external class review findings
* Fix deferred external class method unregistration
* Resolve external class signatures on unload
* Fix external class validation and cleanup
* Fix external-class method hook cleanup
* Fix external class dispatch handling
* Fix deferred external class registration
* Resolve external signatures on method unregister
* Check external class versions before string match
* Remove stale S7 hooks after failed loads
* Reject package-less external class matches
* Normalize external generic sentinels
* Register runtime external-class methods
* Fix deferred external class method registration
* Fix external generic hook replacement
* Short-circuit union class checks
* Address external method registration reviews
* Fix external class union handling
* Remove stale deferred external methods
* Fix external method unregister review cases
* Fix deferred union registration
* Preserve versioned external generic on unregister
* Allow external classes to narrow S7_object
* Simplify external class matching
* Simplify external class cleanup
* Simplify external class registration
* Trim external class changes
* Trim external class cleanup
* Simplify external class resolution
* Simplify external dependency checks
* Use closure for external class validation
* Remove external S7_object special case
* Document generic sentinel invariant
* Trim-external-class-tests
* Clarify-external-package-vignette
* Clarify external class resolution errors
* Restore external class docs example
* Restore external dependency flattening helper
* Simplify external class error construction
* Snapshot external class binding mismatches
* Restore external class default snapshot
* Reduce external class snapshot churn
* Fix external class review findings
[P2] Allow subclasses of external parent classes — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:372-375: Valid narrowing is rejected when a child class is a real S7 subclass of an external parent class. The branch returns FALSE unless the child has the exact same package/name as the external placeholder, so cases like dep::Sub extending dep::Base cannot override a parent property typed as new_external_class("dep", "Base") even though the inheritance chain would make it valid.
[P2] Remove methods when an external class unloads first — /Users/tomasz/github/RConsortium/S7/R/hooks.R:89-90: If the external class package unloads before the downstream package, external_deps_resolvable(deps) is false and unload uses the placeholder signature. Since the method was registered with resolved class objects, unregister_own_S7_method() builds a non-identical method and leaves the downstream method installed in the upstream generic after S7_on_unload().
* Rename `S7_class` attribute to `_S7_class` (#689)
This keeps the name as similar as possible, but makes it clear that it's in the namespace S7 reserves for its own usage. Includes backward compatibility fallback that we can remove in a future version.
Closes #677
* Clarify external class resolution errors
* avoid binding `getNamespaceVersion` in package namespace
* Refactor test helpers to use code blocks (#714)
---------
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
S7 >= 0.3.0 renames the `S7_class` attribute to `_S7_class` (RConsortium/S7#689). Try `_S7_class` first and fall back to `S7_class` for backward compatibility with older S7 versions. Closes #1797
3 participants
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