I might add that the old (very old) style in Biobase was to use "new" as constructor. That one should avoid this all the time, is a newer insight, so you'll find plenty of old code violating this.
Kasper On Mon, Jan 26, 2015 at 8:35 AM, Hahne, Florian <florian.ha...@novartis.com> wrote: > Thanks, > not much need for convincing here, just wanted a big enough push to get me > over that 'kinda works so why bother` hurdle. > I need to do other code refactoring in Gviz soon, and will take the > opportunity to kick out all initialisers in the process. So one less > package to worry about. > Florian > > On 26/01/15 14:28, "Michael Lawrence" <lawrence.mich...@gene.com> wrote: > > >In the S4Vectors/IRanges infrastructure, we have never defined an > >initialize method, so it is certainly possible to use the constructor > >pattern in complex frameworks, and IRanges serves as a good example of > >doing so. The basic pattern is to delegate to the superclass > >constructor and pass that object as an unnamed argument to new(). > > > >We stayed away from changing the formals of initialize, because (a) > >initialize() has a special contract of no-arg invocation that is a > >pain to support and may not be consistent with the user-facing > >interface and (b) more importantly, we wanted to preserve the ability > >(at the low level) to re-initialize based purely on slots. Once you > >have a custom initialize, it is no longer possible to call new() > >consistently (just with slots) across all classes. And (c), since in > >general we want constructors anyway (the user calling new() directly > >would sacrifice abstraction), this policy greatly reduced complexity > >by keeping the logic at a single layer. Others might have more to add; > >I stopped thinking after I got to three ;) > > > >Hope that helps, > >Michael > > > > > > > > > > > >On Mon, Jan 26, 2015 at 12:38 AM, Hahne, Florian > ><florian.ha...@novartis.com> wrote: > >> Hi Michael, > >> I'll take a look. In order to improve my code: what exactly do you think > >> should be part of the initialiser, and what should be in the > >>constructor? > >> There don't seem to bee any clear guidelines out there anywhere. And if > >> all logic goes in the constructor, how does one deal with more complex > >> nested inheritance? And what's the use for the initialiser in the first > >> place? > >> Florian > >> > >> > >> On 24/01/15 03:03, "Michael Lawrence" <lawrence.mich...@gene.com> > wrote: > >> > >>>I have found and fixed the affected initialize cases and will begin > >>>checking in fixes. > >>> > >>>Affected packages: RDAVIDWebService, flowCore (as we know), Gviz, > >>>ChIPseqR, Pviz, VanillaICE, flowFit. > >>> > >>>As an aside, in all of these cases, initialize is implementing logic > >>>that really belongs in a constructor function. Treating new() as a > >>>high-level constructor should be discouraged in my opinion. Has > >>>nothing to do with this bug though. > >>> > >>>I have also begun looking through cases outside of initialize. There > >>>are only about 300 cases of callNextMethod() in the codebase. Great > >>>majority seem OK so far. > >>> > >>> > >>>On Fri, Jan 23, 2015 at 12:46 PM, Michael Lawrence <micha...@gene.com> > >>>wrote: > >>>> That's right. > >>>> > >>>> On Fri, Jan 23, 2015 at 12:22 PM, Mike <wjia...@fhcrc.org> wrote: > >>>>> Sorry, I just want to clarify some more on this.(maybe useful for > >>>>>others as > >>>>> well) > >>>>> What you actually mean is callNextMethod() used to drop both ... and > >>>>>the > >>>>> other arguments explicitly supplied from the parent call (in my case, > >>>>> parameters argument). > >>>>> And now after your fix, both gets passed on and that¹s why I should > >>>>> explicitly select the argument for callNextMethod? > >>>>> > >>>>> Thanks. > >>>>> > >>>>> Mike > >>>>> > >>>>> On 01/23/2015 11:30 AM, Michael Lawrence wrote: > >>>>> > >>>>> The bug has existed forever. The commit log may be confusing. The > >>>>> actual bug (or at least a very undesirable behavior) was in > >>>>> match.call(), not callNextMethod(). > >>>>> > >>>>> I think it's still true that callNextMethod() is the natural > >>>>> invocation. When adding arguments to initialize that you do not want > >>>>> to pass on (and thus set as slots), it's necessary to use explicit > >>>>> args. There are other cases where callNextMethod() is exactly what > >>>>>you > >>>>> want. Like the case in rtracklayer that motivated this fix. > >>>>> > >>>>> > >>>>> On Fri, Jan 23, 2015 at 11:23 AM, Mike <wjia...@fhcrc.org> wrote: > >>>>> > >>>>> Michael, > >>>>> > >>>>> Thanks for the confirmation of the issue. I see you were trying to > >>>>>tackle it > >>>>> in the latest commits r67467:67472 which apparently haven¹t fixed the > >>>>>bug > >>>>> yet (instead it triggers the error of the existing legacy code in > >>>>>other R > >>>>> packages like flowCore). I can certainly change the code to > >>>>>explicitly > >>>>>pass > >>>>> on all the arguments to callNextMethod as you and Martin suggested. I > >>>>>just > >>>>> wonder if the documentation should drop the sentence of Calling with > >>>>>no > >>>>> arguments is often the natural way to use callNextMethod and change > >>>>>the > >>>>> example code (at least before the bug is eventually fixed.) so that > >>>>>users > >>>>> won¹t be misusing it. > >>>>> > >>>>> > >>>>> > >>>>> Mike > >>>>> > >>>>> On 01/23/2015 10:55 AM, Martin Morgan wrote: > >>>>> > >>>>> On 01/23/2015 10:52 AM, Michael Lawrence wrote: > >>>>> > >>>>> First let me apologize for my failure to read emails. There was a > >>>>> long-standing bug in the methods package where arguments passed as > >>>>> "..." to a method would be dropped by callNextMethod(). It turns out > >>>>> that in all known cases this affects calls to initialize(), probably > >>>>> because there are many initialize() methods, and new() calls > >>>>> initialize with "...". > >>>>> > >>>>> This case is a very typical one, and Martin's fix is the right one, > >>>>> according to the (unchanged) documentation of callNextMethod(). > >>>>> > >>>>> It's in general impossible to detect these cases from static > >>>>>analysis, > >>>>> since we do not know how the user is calling a method. But catching > >>>>> them in initialize() should be easy, with some false positives. Just > >>>>> look for callNextMethod(). > >>>>> > >>>>> Is it OK for me to checkout all of Bioconductor so that I can perform > >>>>> that analysis, or will that stress the servers too much? I have a > >>>>> package that embeds GNU Global to index and search > >>>>> CRAN/Bioconductor-size repositories for these cases. > >>>>> > >>>>> > >>>>> Hi Michael -- there is no problem checking out all > >>>>> (https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks > >>>>>presumably) of > >>>>> Bioc. > >>>>> > >>>>> Thanks! Martin > >>>>> > >>>>> > >>>>> Michael > >>>>> > >>>>> On Thu, Jan 22, 2015 at 6:15 AM, Martin Morgan > >>>>><mtmor...@fredhutch.org> > >>>>> wrote: > >>>>> > >>>>> On 01/22/2015 12:26 AM, Martin Maechler wrote: > >>>>> > >>>>> > >>>>> Mike <wjia...@fhcrc.org> > >>>>> on Tue, 20 Jan 2015 17:16:37 -0800 writes: > >>>>> > >>>>> > >>>>> > >>>>> > I don't think it has been addressed yet in the later commits > >>>>>of > >>>>> R-devel. > >>>>> > And that piece of code in flowCore package was written long > >>>>>time > >>>>> ago and > >>>>> > there is nothing wrong with it as far as I can see. > >>>>> > >>>>> You are right. > >>>>> > >>>>> I thought Michael Lawrence (member of R Core since last summer!) > >>>>> was also reading Bioc-devel, so wonder why he has not yet > >>>>> replied --> CC'ing him > >>>>> > >>>>> The changes to R-devel also did break the Matrix package in > >>>>> exactly the same way. You said > >>>>> > >>>>> Here is the |initialize|method for |parameterFilter| which causes the > >>>>> various errors from flow package vignettes. > >>>>> > >>>>> |setMethod("initialize", > >>>>> signature=signature(.Object="parameterFilter"), > >>>>> definition=function(.Object, parameters,...) > >>>>> { > >>>>> if (!missing(parameters)) > >>>>> parameters(.Object) <- parameters > >>>>> callNextMethod() > >>>>> }) > >>>>> | > >>>>> > >>>>> > >>>>> > >>>>> and I also had a _no argument_ call > >>>>> callNextMethod() > >>>>> inside an initialize method. > >>>>> > >>>>> I'm pretty sure that if you change (the same as I) > >>>>> > >>>>> callNextMethod() > >>>>> to > >>>>> callNextMethod(.Object, ...) > >>>>> > >>>>> you'll have a version of the code that works both in current R 3.1.2 > >>>>> (and older versions) *and* in the R-devel version. > >>>>> > >>>>> > >>>>> I also pinged Michael?? > >>>>> > >>>>> What's a precise statement of the problem -- no-argument > >>>>>callNextMethod() > >>>>> inside initialize? Any suggestions on ways to discover these without > >>>>>relying > >>>>> on package break during build / check / install? > >>>>> > >>>>> Martin Morgan > >>>>> > >>>>> Michael L and I were not sure how many other packages or R code this > >>>>> would influence and he was expecting very very few. > >>>>> > >>>>> Best regards, > >>>>> > >>>>> Martin Maechler, ETH Zurich > >>>>> > >>>>> > >>>>> > On 01/20/2015 05:06 PM, Dan Tenenbaum wrote: > >>>>> >> I'm not sure if you were implying that this has already been > >>>>>fixed > >>>>> in R-devel. Note that the devel build machines currently have r67501 > >>>>> installed, which is later than all the commits you cite above, yet > >>>>>the > >>>>>flow > >>>>> packages are still broken. > >>>>> > >>>>> _______________________________________________ > >>>>> Bioc-devel@r-project.org mailing list > >>>>> https://stat.ethz.ch/mailman/listinfo/bioc-devel > >>>>> > >>>>> > >>>>> > >>>>> -- > >>>>> Computational Biology / Fred Hutchinson Cancer Research Center > >>>>> 1100 Fairview Ave. N. > >>>>> PO Box 19024 Seattle, WA 98109 > >>>>> > >>>>> Location: Arnold Building M1 B861 > >>>>> Phone: (206) 667-2793 > >>>>> > >>>>> > >>>>> > >>>>> > >>> > >>>_______________________________________________ > >>>Bioc-devel@r-project.org mailing list > >>>https://stat.ethz.ch/mailman/listinfo/bioc-devel > >> > >> _______________________________________________ > >> Bioc-devel@r-project.org mailing list > >> https://stat.ethz.ch/mailman/listinfo/bioc-devel > > _______________________________________________ > Bioc-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/bioc-devel > [[alternative HTML version deleted]] _______________________________________________ Bioc-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/bioc-devel
