I find your arguments fairly reasonable. I wonder if you have been through the process of restoring data from an object serialized with the old slot names, using the revised package. Maybe it is not even a use case for your packages. If you have worked through the implications of this change for all dependent packages, and since the policy on slot names is not clearly articulated, I would be inclined to allow the change -- but I would like to hear from other folks on bioc-devel and in our core.
On Fri, Oct 15, 2021 at 1:51 PM Chris Eeles <christopher.ee...@outlook.com> wrote: > Hi Vincent, > > Thanks for sharing your thoughts. > > > > I guess my thinking was that the entire point of using S4 classes and OOP > is having accessor methods to provide an implementation independent API. In > the Bioconductor guidelines it specifically tells users not to access slots > using the `@` operator, as an implementation change in the class may break > any scripts doing so. Therefore, my changing slot names should have no > effect on users following the Bioconductor coding best practices, assuming > I maintain the old accessors methods with a .Deprecation warning, as per > the cited guideline. That was indeed my plan. > > So I would argue that no, class definitions are not part of the API, > especially if I am just renaming slots. Indeed isn’t that one of the > supposed strengths of OOP programming and the use of interfaces? > > Obviously I have already agreed to wait for 3.15 to make the changes, but > I do not think it is clear from the current guidelines that deprecation > rules apply to slots. Given that `@` isn’t even a generic, there would be > no way to send a message to the user except through the accessor methods, > which they would never see if they weren’t already using the accessor API. > So for users accessing data via `@`, the deprecation guidelines provide no > benefits because they failed to follow the best practices. > > My opinion of the developer-user contract for S4 classes is that the API > would not change without due warning, and if implementation really is > independent of interface, then any changes made to an S4 class should be > fine, so long as all the original methods still work and can be deprecated > according to the cited guidelines. > > Additionally, if changes to a class require so much work, it incentives > developers to simply ditch old S4 classes and reimplement them in a new > package. Doesn’t that go against the spirit of reuse that is supposed to be > encouraged by adoption of S4 classes? > > TL;DR – IMO API = interface; implementation is developer business > > Best, > > Chris > > > > *From:* Vincent Carey <st...@channing.harvard.edu> > *Sent:* October 15, 2021 1:31 PM > *To:* Chris Eeles <christopher.ee...@outlook.com> > *Cc:* Hervé Pagès <hpages.on.git...@gmail.com>; bioc-devel@r-project.org > *Subject:* Re: [Bioc-devel] Best Practices for Renaming S4 Slots > > > > I will defer to Herve about all details, but I would say that this level > of change control is implied by the "no changes > > to package API without an interval of deprecation spanning at least one > release". See https://bioconductor.org/developers/how-to/deprecation/ > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbioconductor.org%2Fdevelopers%2Fhow-to%2Fdeprecation%2F&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846578705%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1vILqDKSUJPMJ5gKZ1y%2Ftlf8rKtrZmX6tL6xGTINEo8%3D&reserved=0> > > That text mentions that removal may take 18 months. > > > > Whatever is exposed cannot be changed without a deprecation period, > > in which the functionality is preserved, but notification is given to > users/developers, through .Deprecated, that > > functionality will change and advice is given on the alternate approach to > be used. > > > > Is a slot name part of the API? It isn't completely obvious, but in the > case of serialized objects, it turns out that it is. > > I don't know that our guidelines have sufficient details on this process, > but we welcome your input on where to > > best outline/advertise this. > > > > On Fri, Oct 15, 2021 at 1:22 PM Chris Eeles <christopher.ee...@outlook.com> > wrote: > > Message received. I will leave that branch for later. Is this information > available on the Bioconductor website at all? It would have been useful to > find out sooner. > > Best, > Chris > > -----Original Message----- > From: Bioc-devel <bioc-devel-boun...@r-project.org> On Behalf Of Chris > Eeles > Sent: October 15, 2021 1:10 PM > To: Hervé Pagès <hpages.on.git...@gmail.com>; bioc-devel@r-project.org > Subject: Re: [Bioc-devel] Best Practices for Renaming S4 Slots > > Thanks Herve, > > I actually got the updateObject method working after sending this email, > but thanks for the information. Maybe it is worth adding a section on this > topic to the Bioconductor developer section? > > Unfortunately, I was unaware that the start of development cycle was the > best time to implement this change. I am currently planning to have this > done for the 3.14 release. > > I am introducing new accessors as well but keeping the old ones for > backwards compatibility using aliases. > > How discouraged are slot name changes in a release? A lot of the changes > on our road map require the slots to be renamed so it would significantly > delay required features if I were to wait. > > I plan to put in the work so that those using accessors shouldn't notice a > difference. > > Best, > Chris > > -----Original Message----- > From: Hervé Pagès <hpages.on.git...@gmail.com> > Sent: October 15, 2021 12:39 PM > To: Chris Eeles <christopher.ee...@outlook.com>; bioc-devel@r-project.org > Subject: Re: [Bioc-devel] Best Practices for Renaming S4 Slots > > Hi Chris, > > There was some formatting issues with my previous answer so I'm sending it > again. Hopefully this time the formatting is better. See below. > > On 14/10/2021 13:08, Chris Eeles wrote: > > Hello BioC community, > > > > I am the lead developer for the CoreGx, PharmacoGx, RadioGx and ToxicoGx > packages. We have recently been working on major updates to the structure > of a CoreSet, which is inherited by the main class in all three of the > other packages listed. > > > > While making these changes, we would like to rename some CoreSet slots > to increase the amount of code that can be refactored into CoreGx, > simplifying maintenance and development of inheriting packages in the > future. > > > > The slot names and their accessors will be made more generic, for > example the 'cell' slot will become 'sample' to allow a CoreSet derived > class to store Biological model systems other than cancer cell lines. > Similarly, 'drug' or 'radiation' slots in inheriting packages will be > replaced with a 'treatment' slot in the CoreSet. This will allow us to > simplify inheritance, removing much redundant code from the inheriting > packages and setting us up to integrate other lab packages, such as Xeva > for PDX models, into the general CoreSet infrastructure. > > > > I took a brief look through the Bioconductor developer documentation but > didn't see anything talking about best practices for renaming slots. > > > > It is easy enough to make the code changes, but my major concern is > being able to update existing objects from these packages to use the new > slot names. > > > > I am aware of the updateObject function in BiocGenerics, but tried using > it to update some example data in CoreGx without success. > > > > Is this the proper function to use when you wish to update an S4 object > whose class definition has been modified? If not, is there existing > infrastructure for accomplishing this task? > > Yes updateObject() is the proper function to use but the function has no > way to guess how to fix your objects. The way you tell it what to do is by > implementing methods for your objects. > > For example if you renamed the 'cell' slot -> 'sample', your > updateObject() method will be something like this: > > > setMethod("updateObject", "CoreSet", > function(object, ..., verbose=FALSE) > { > ## The "cell" slot was renamed -> "sample" in CoreGx_1.7.1. > if (.hasSlot(object, "cell")) { > object <- new("CoreSet", > sensitivity=object@sensitivity, > annotation=object@annotation, > molecularProfiles=object@molecularProfiles, > sample=object@cell, > datasetType=object@datasetType, > perturbation=object@perturbation, > curation=object@curation) > return(object) > } > object > } > ) > > The best time to do this internal renaming is at the beginning of the BioC > 3.15 development cycle (i.e. right after the 3.14 release). > > If in the future, other slots get renamed or added, you'll need to modify > the updateObject() method above like this: > > setMethod("updateObject", "CoreSet", > function(object, ..., verbose=FALSE) > { > ## The "cell" slot was renamed -> "sample" in CoreGx_1.7.1. > if (.hasSlot(object, "cell")) { > object <- new("CoreSet", > sensitivity=object@sensitivity, > annotation=object@annotation, > molecularProfiles=object@molecularProfiles, > sample=object@cell, > datasetType=object@datasetType, > perturbation=object@perturbation, > titi=object@curation) # use "titi" here too! > return(object) > } > ## The "curation" slot was renamed -> "titi" in CoreGx_1.9.1. > if (.hasSlot(object, "curation")) { > object <- new("CoreSet", > sensitivity=object@sensitivity, > annotation=object@annotation, > molecularProfiles=object@molecularProfiles, > sample=object@sample, # use "sample" here! > datasetType=object@datasetType, > perturbation=object@perturbation, > titi=object@curation) > return(object) > } > object > } > ) > > And so on... > > Hope this helps, > H. > > > > > > Any tips for implementing slot renaming, as well as links to existing > documentation or articles on the topic would be appreciated. > > > > Best, > > --- > > Christopher Eeles > > Software Developer > > BHK > > Laboratory<https://na01.safelinks.protection.outlook.com/?url=http%3A% > <https://na01.safelinks.protection.outlook.com/?url=http%3A%25> > > 2F%2Fwww.bhklab.ca > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2F2fwww.bhklab.ca%2F&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846588662%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oQjaDi%2By2aBcR0fDoAq%2FsGw92T4NQKENJF6RX8zb9AA%3D&reserved=0> > %2F&data=04%7C01%7C%7C170e009bdbda4e7f182308d990 > > 000468%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699152031882488 > > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I > > k1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eu7jm6LiWISZ01etllrdipTAkVtI4a7 > > rZumELnt0siI%3D&reserved=0> Princess Margaret Cancer > > Centre<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F% > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%25> > > 2Fwww.pmgenomics.ca > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2F2fwww.pmgenomics.ca%2F&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846588662%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dEu%2Bv1UzsrTRLmjw5bPtEhHEGnYYmGQN%2B9a6PjLLj2E%3D&reserved=0> > %2Fpmgenomics%2F&data=04%7C01%7C%7C170e009bdbda > > 4e7f182308d990000468%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C6376 > > 99152031882488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l > > uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g8gXrDgyJ5YRHdiBv > > q77WiDkCgWcYhWWW9R7OWKMxqw%3D&reserved=0> > > University Health > > Network<https://na01.safelinks.protection.outlook.com/?url=http%3A%2F% > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%25> > > 2Fwww.uhn.ca > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2F2fwww.uhn.ca%2F&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846598621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=muzG7Yz1KliU9n%2FNpQXkfj4oNpJqFnnrPlD6ae81BAk%3D&reserved=0> > %2F&data=04%7C01%7C%7C170e009bdbda4e7f182308d990000468 > > %7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699152031882488%7CUnk > > nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw > > iLCJXVCI6Mn0%3D%7C1000&sdata=IJKtucTmctj0z227jGpqnBeZ0D9ruvODNLkmT > > QBdX%2Bs%3D&reserved=0> > > > > > > [[alternative HTML version deleted]] > > > > _______________________________________________ > > Bioc-devel@r-project.org mailing list > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstat. > > ethz.ch > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fethz.ch%2F&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846608572%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=MbzF83IFpbNGcb9HFG8q6TnPUeCfeXB8HU%2BsDscmuCo%3D&reserved=0> > %2Fmailman%2Flistinfo%2Fbioc-devel&data=04%7C01%7C%7C170e00 > > 9bdbda4e7f182308d990000468%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0% > > 7C637699152031882488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI > > joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nNq88Xhpby% > > 2FVJxZ%2BdBWPk72Cp%2FS3EsaFgQ3FhrkaaH4%3D&reserved=0 > > > > -- > Hervé Pagès > > Bioconductor Core Team > hpages.on.git...@gmail.com > > _______________________________________________ > Bioc-devel@r-project.org mailing list > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstat.ethz.ch%2Fmailman%2Flistinfo%2Fbioc-devel&data=04%7C01%7C%7C170e009bdbda4e7f182308d990000468%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699152031882488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nNq88Xhpby%2FVJxZ%2BdBWPk72Cp%2FS3EsaFgQ3FhrkaaH4%3D&reserved=0 > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstat.ethz.ch%2Fmailman%2Flistinfo%2Fbioc-devel&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846608572%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zNC1AVCelVynnCTWncoEeARzFx%2B%2BX3PKQ0WO%2Fp5X%2F5w%3D&reserved=0> > > _______________________________________________ > Bioc-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/bioc-devel > <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstat.ethz.ch%2Fmailman%2Flistinfo%2Fbioc-devel&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846618526%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uyYpbCJlKPCS3EQVZR8vEQaRj6btOUkJXs2qOOCWonU%3D&reserved=0> > > > The information in this e-mail is intended only for the person to whom it > is > addressed. If you believe this e-mail was sent to you in error and the > e-mail > contains patient information, please contact the > Partners Compliance HelpLine at > http://www.partners.org/complianceline > <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.partners.org%2Fcomplianceline&data=04%7C01%7C%7C8ec56a4ffda74802e7ab08d990019bc7%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637699158846618526%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fxCSoQPYoR9LPKcF%2BW6Khuk4LHgJ%2B37%2B5mNDwKFustQ%3D&reserved=0> > . > If the e-mail was sent to you in error > but does not contain patient information, please contact the sender and > properly > dispose of the e-mail. > -- The information in this e-mail is intended only for the person to whom it is addressed. If you believe this e-mail was sent to you in error and the e-mail contains patient information, please contact the Partners Compliance HelpLine at http://www.partners.org/complianceline <http://www.partners.org/complianceline> . If the e-mail was sent to you in error but does not contain patient information, please contact the sender and properly dispose of the e-mail. [[alternative HTML version deleted]] _______________________________________________ Bioc-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/bioc-devel
