>>>>> Duncan Murdoch writes:> On 12/06/2015 7:16 AM, Kurt Hornik wrote: >>>>>>> Duncan Murdoch writes: >> >>> On 12/06/2015 4:12 AM, Martin Maechler wrote: >>>> This is a topic ' "apparent S3 methods" note in R CMD check ' >>>> from R-package-devel >>>> https://stat.ethz.ch/pipermail/r-package-devel/2015q2/000126.html >>>> >>>> which is relevant to here because some of us have been thinking >>>> about extending R because of the issue. >>>> >>>> John Fox, maintainer of the 'effects' package has enquired about >>>> the following output from 'R CMD check effects' >>>> >>>>> * checking S3 generic/method consistency ... NOTE >>>>> Found the following apparent S3 methods exported but not registered: >>>>> all.effects >>>> >>>> and added >>>> >>>>> The offending function, all.effects(), is deprecated in favour of >>>>> allEffects(), but I'd rather not get rid of it for backwards compatibility. >>>>> Is there any way to suppress the note without removing all.effects()? >>>> >>>> and I had agreed that this was a "False Positive" in this case. >>>> >>>> [.......] >>>> >>>> and then >>>> >>>>> Now I agree .. and have e-talked about this with another R core >>>>> member .. that it would be desirable for the package author to >>>>> effectively declare the fact that such a function is not an S3 >>>>> method even though it "looks like it" at least if looked from far. >>>> >>>>> So, ideally, you could have something like >>>> >>>>> nonS3method("all.effects") >>>> >>>>> somewhere in your package source ( in NAMESPACE or R/*.R ) >>>>> which would tell the package-checking code -- but *ALSO* all the other S3 >>>>> method code that all.effects should be treated as a regular R >>>>> function. >>>> >>>>> I would very much like such a feature in R, and for that reason, >>>>> I'm cross posting this (as one of the famous exceptions that >>>>> accompany real-life rules!!) to R-devel. >>>> >>>> and actually I did *not* cross post, but have now moved the >>>> relevant part of the thread to R-devel. >>>> >> >>> It sounds like a good idea. It's a nontrivial amount of work, because >>> of the "all the other S3 method code" part. There's the question of >>> functions defined outside of packages: presumably they are still S3 >>> methods, with no way to suppress that. >> >> I am not sure this is the right solution: S3 dispatch will still occur >> because we first look at foo.bar exports and then in the S3 registry, >> afaicr (the "all the other S3 method code" part). >> >> If we could move to only looking at the registry for dispatch, there >> would be no need to declare situations where we should not dispatch on >> foo.bar exports. >>> I think that would break a lot of existing scripts. I think the logic > should be something like this.> For each class in the class list:> Search backwards through the environment chain. If the current location > is a package environment or namespace, look only in the registry. If > not, look at all functions.> If that search failed, try the next class.Yep---that's what I meant. I forgot to write the "if namespace semantics applies" part :-) Best -k> A variation on the test is: If there's a registry in the current > location, look there. But I think the registry is not on the search > list, so I'm not sure that would work.> This assumes that we keep separate registries in each package; if we > merge them into one big registry, it gets harder. I'm not familiar > enough with the code to know which way we do it.> Duncan Murdoch
To me, it seems like there's actually two problems here: 1) Preventing all() from dispatching to all.effects() for objects of class effects 2) Eliminating the NOTE in R CMD check My impression is that 1) actually causes few problems, particularly since people are mostly now aware of the problem and avoid using `.` in function names unless they're S3 methods. Fixing this issue seems like it would be a lot of work for relatively little gain. However, I think we want to prevent people from writing new functions with this confusing naming scheme, but equally we want to grandfather in existing functions, because renaming them all would be a lot of work (I'm looking at you t.test()!). Could we have a system similar to globalVariables() where you could flag a function as definitely not being an S3 method? I'm not sure what R CMD check should do - ideally you wouldn't be allow to use method.class for new functions, but still be able suppress the note for old functions that can't easily be changed. Hadley On Fri, Jun 12, 2015 at 6:52 AM, Kurt Hornik <Kurt.Hornik at wu.ac.at> wrote:>>>>>> Duncan Murdoch writes: > >> On 12/06/2015 7:16 AM, Kurt Hornik wrote: >>>>>>>> Duncan Murdoch writes: >>> >>>> On 12/06/2015 4:12 AM, Martin Maechler wrote: >>>>> This is a topic ' "apparent S3 methods" note in R CMD check ' >>>>> from R-package-devel >>>>> https://stat.ethz.ch/pipermail/r-package-devel/2015q2/000126.html >>>>> >>>>> which is relevant to here because some of us have been thinking >>>>> about extending R because of the issue. >>>>> >>>>> John Fox, maintainer of the 'effects' package has enquired about >>>>> the following output from 'R CMD check effects' >>>>> >>>>>> * checking S3 generic/method consistency ... NOTE >>>>>> Found the following apparent S3 methods exported but not registered: >>>>>> all.effects >>>>> >>>>> and added >>>>> >>>>>> The offending function, all.effects(), is deprecated in favour of >>>>>> allEffects(), but I'd rather not get rid of it for backwards compatibility. >>>>>> Is there any way to suppress the note without removing all.effects()? >>>>> >>>>> and I had agreed that this was a "False Positive" in this case. >>>>> >>>>> [.......] >>>>> >>>>> and then >>>>> >>>>>> Now I agree .. and have e-talked about this with another R core >>>>>> member .. that it would be desirable for the package author to >>>>>> effectively declare the fact that such a function is not an S3 >>>>>> method even though it "looks like it" at least if looked from far. >>>>> >>>>>> So, ideally, you could have something like >>>>> >>>>>> nonS3method("all.effects") >>>>> >>>>>> somewhere in your package source ( in NAMESPACE or R/*.R ) >>>>>> which would tell the package-checking code -- but *ALSO* all the other S3 >>>>>> method code that all.effects should be treated as a regular R >>>>>> function. >>>>> >>>>>> I would very much like such a feature in R, and for that reason, >>>>>> I'm cross posting this (as one of the famous exceptions that >>>>>> accompany real-life rules!!) to R-devel. >>>>> >>>>> and actually I did *not* cross post, but have now moved the >>>>> relevant part of the thread to R-devel. >>>>> >>> >>>> It sounds like a good idea. It's a nontrivial amount of work, because >>>> of the "all the other S3 method code" part. There's the question of >>>> functions defined outside of packages: presumably they are still S3 >>>> methods, with no way to suppress that. >>> >>> I am not sure this is the right solution: S3 dispatch will still occur >>> because we first look at foo.bar exports and then in the S3 registry, >>> afaicr (the "all the other S3 method code" part). >>> >>> If we could move to only looking at the registry for dispatch, there >>> would be no need to declare situations where we should not dispatch on >>> foo.bar exports. >>> > >> I think that would break a lot of existing scripts. I think the logic >> should be something like this. > >> For each class in the class list: > >> Search backwards through the environment chain. If the current location >> is a package environment or namespace, look only in the registry. If >> not, look at all functions. > >> If that search failed, try the next class. > > Yep---that's what I meant. I forgot to write the "if namespace > semantics applies" part :-) > > Best > -k > >> A variation on the test is: If there's a registry in the current >> location, look there. But I think the registry is not on the search >> list, so I'm not sure that would work. > >> This assumes that we keep separate registries in each package; if we >> merge them into one big registry, it gets harder. I'm not familiar >> enough with the code to know which way we do it. > >> Duncan Murdoch > > ______________________________________________ > R-devel at r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel-- http://had.co.nz/
>>>>> Hadley Wickham <h.wickham at gmail.com> >>>>> on Fri, 12 Jun 2015 09:53:20 -0500 writes:> To me, it seems like there's actually two problems here: > 1) Preventing all() from dispatching to all.effects() for objects of > class effects > 2) Eliminating the NOTE in R CMD check Sure. ... and that what I said in my OP > My impression is that 1) actually causes few problems, particularly > since people are mostly now aware of the problem and avoid using `.` > in function names unless they're S3 methods. Fixing this issue seems > like it would be a lot of work for relatively little gain. Well, we may disagree here. I did mean to improve the system "deep down", and Kurt and Duncan where basically also blowing the same trumpet. We are not re-inventing all the wheels on all the cars that have been racing around - sometimes for decennia... and so some names have been there for a long time and will remain. > However, I think we want to prevent people from writing new functions > with this confusing naming scheme, but equally we want to grandfather > in existing functions, because renaming them all would be a lot of > work (I'm looking at you t.test()!). exactly. > Could we have a system similar to globalVariables() where you could > flag a function as definitely not being an S3 method? did you read what I wrote originally? Exactly that - with a working-proposition nonS3method("<name>") And BTW ---- I'm diverting and I seem to "preach" in vain here, but in my view globalVariables("xy") is really a very very poor "solution" and far from what one would desire: If you use it in your package, say because one function uses non-standard evaluation and hence 'xy', *all* erronous global uses of 'xy in all other functions in your package will *not* be flagged by the nice codetools package functionality which is behind that part of R CMD check. Martin > I'm not sure what R CMD check should do - ideally you > wouldn't be allow to use method.class for new functions, > but still be able suppress the note for old functions that > can't easily be changed. > Hadley > On Fri, Jun 12, 2015 at 6:52 AM, Kurt Hornik <Kurt.Hornik at wu.ac.at> wrote: >>>>>>> Duncan Murdoch writes: >> >>> On 12/06/2015 7:16 AM, Kurt Hornik wrote: >>>>>>>>> Duncan Murdoch writes: >>>> >>>>> On 12/06/2015 4:12 AM, Martin Maechler wrote:>>>>> This is a topic ' "apparent S3 methods" note in R CMD check ' >>>>> from R-package-devel >>>>> https://stat.ethz.ch/pipermail/r-package-devel/2015q2/000126.html>>>>>>>>>>> which is relevant to here because some of us have been thinking >>>>> about extending R because of the issue.>>>>>>>>>>> John Fox, maintainer of the 'effects' package has enquired about >>>>> the following output from 'R CMD check effects'>>>>>> >>>>>>> * checking S3 generic/method consistency ... NOTE >>>>>>> Found the following apparent S3 methods exported but not registered: >>>>>>> all.effects >>>>>>>>>>> and added>>>>>> >>>>>>> The offending function, all.effects(), is deprecated in favour of >>>>>>> allEffects(), but I'd rather not get rid of it for backwards compatibility. >>>>>>> Is there any way to suppress the note without removing all.effects()? >>>>>>>>>>> and I had agreed that this was a "False Positive" in this case.>>>>>>>>>>> [.......]>>>>>>>>>>> and then>>>>>> >>>>>>> Now I agree .. and have e-talked about this with another R core >>>>>>> member .. that it would be desirable for the package author to >>>>>>> effectively declare the fact that such a function is not an S3 >>>>>>> method even though it "looks like it" at least if looked from far. >>>>>> >>>>>>> So, ideally, you could have something like >>>>>> >>>>>>> nonS3method("all.effects") >>>>>> >>>>>>> somewhere in your package source ( in NAMESPACE or R/*.R ) >>>>>>> which would tell the package-checking code -- but *ALSO* all the other S3 >>>>>>> method code that all.effects should be treated as a regular R >>>>>>> function. >>>>>> >>>>>>> I would very much like such a feature in R, and for that reason, >>>>>>> I'm cross posting this (as one of the famous exceptions that >>>>>>> accompany real-life rules!!) to R-devel. >>>>>>>>>>> and actually I did *not* cross post, but have now moved the >>>>> relevant part of the thread to R-devel.>>>>>> >>>> >>>>> It sounds like a good idea. It's a nontrivial amount of work, because >>>>> of the "all the other S3 method code" part. There's the question of >>>>> functions defined outside of packages: presumably they are still S3 >>>>> methods, with no way to suppress that. >>>> >>>> I am not sure this is the right solution: S3 dispatch will still occur >>>> because we first look at foo.bar exports and then in the S3 registry, >>>> afaicr (the "all the other S3 method code" part). >>>> >>>> If we could move to only looking at the registry for dispatch, there >>>> would be no need to declare situations where we should not dispatch on >>>> foo.bar exports. >>>> >> >>> I think that would break a lot of existing scripts. I think the logic >>> should be something like this. >> >>> For each class in the class list: >> >>> Search backwards through the environment chain. If the current location >>> is a package environment or namespace, look only in the registry. If >>> not, look at all functions. >> >>> If that search failed, try the next class. >> >> Yep---that's what I meant. I forgot to write the "if namespace >> semantics applies" part :-) >> >> Best >> -k >> >>> A variation on the test is: If there's a registry in the current >>> location, look there. But I think the registry is not on the search >>> list, so I'm not sure that would work. >> >>> This assumes that we keep separate registries in each package; if we >>> merge them into one big registry, it gets harder. I'm not familiar >>> enough with the code to know which way we do it. >> >>> Duncan Murdoch >> >> ______________________________________________ >> R-devel at r-project.org mailing list >> https://stat.ethz.ch/mailman/listinfo/r-devel > -- > http://had.co.nz/
On 12/06/2015 10:53 AM, Hadley Wickham wrote:> To me, it seems like there's actually two problems here: > > 1) Preventing all() from dispatching to all.effects() for objects of > class effects > 2) Eliminating the NOTE in R CMD check > > My impression is that 1) actually causes few problems, particularly > since people are mostly now aware of the problem and avoid using `.` > in function names unless they're S3 methods. Fixing this issue seems > like it would be a lot of work for relatively little gain. > > However, I think we want to prevent people from writing new functions > with this confusing naming scheme, but equally we want to grandfather > in existing functions, because renaming them all would be a lot of > work (I'm looking at you t.test()!). > > Could we have a system similar to globalVariables() where you could > flag a function as definitely not being an S3 method? I'm not sure > what R CMD check should do - ideally you wouldn't be allow to use > method.class for new functions, but still be able suppress the note > for old functions that can't easily be changed.We have a mechanism for suppressing the warning for existing functions, it's just not available to users to modify. So it would be possible to add effects::all.effects to the stop list, and this might be the easiest action here. This isn't perfect because all.effects() would still act as a method. However, it does give the deprecated message if you ever call it, so nobody would do this unknowingly. The only real risk is that if anyone ever wrote an all.effects function that *was* supposed to be an S3 method, it might be masked by the one in effects. Duncan Murdoch> > Hadley > > On Fri, Jun 12, 2015 at 6:52 AM, Kurt Hornik <Kurt.Hornik at wu.ac.at> wrote: >>>>>>> Duncan Murdoch writes: >> >>> On 12/06/2015 7:16 AM, Kurt Hornik wrote: >>>>>>>>> Duncan Murdoch writes: >>>> >>>>> On 12/06/2015 4:12 AM, Martin Maechler wrote: >>>>>> This is a topic ' "apparent S3 methods" note in R CMD check ' >>>>>> from R-package-devel >>>>>> https://stat.ethz.ch/pipermail/r-package-devel/2015q2/000126.html >>>>>> >>>>>> which is relevant to here because some of us have been thinking >>>>>> about extending R because of the issue. >>>>>> >>>>>> John Fox, maintainer of the 'effects' package has enquired about >>>>>> the following output from 'R CMD check effects' >>>>>> >>>>>>> * checking S3 generic/method consistency ... NOTE >>>>>>> Found the following apparent S3 methods exported but not registered: >>>>>>> all.effects >>>>>> >>>>>> and added >>>>>> >>>>>>> The offending function, all.effects(), is deprecated in favour of >>>>>>> allEffects(), but I'd rather not get rid of it for backwards compatibility. >>>>>>> Is there any way to suppress the note without removing all.effects()? >>>>>> >>>>>> and I had agreed that this was a "False Positive" in this case. >>>>>> >>>>>> [.......] >>>>>> >>>>>> and then >>>>>> >>>>>>> Now I agree .. and have e-talked about this with another R core >>>>>>> member .. that it would be desirable for the package author to >>>>>>> effectively declare the fact that such a function is not an S3 >>>>>>> method even though it "looks like it" at least if looked from far. >>>>>> >>>>>>> So, ideally, you could have something like >>>>>> >>>>>>> nonS3method("all.effects") >>>>>> >>>>>>> somewhere in your package source ( in NAMESPACE or R/*.R ) >>>>>>> which would tell the package-checking code -- but *ALSO* all the other S3 >>>>>>> method code that all.effects should be treated as a regular R >>>>>>> function. >>>>>> >>>>>>> I would very much like such a feature in R, and for that reason, >>>>>>> I'm cross posting this (as one of the famous exceptions that >>>>>>> accompany real-life rules!!) to R-devel. >>>>>> >>>>>> and actually I did *not* cross post, but have now moved the >>>>>> relevant part of the thread to R-devel. >>>>>> >>>> >>>>> It sounds like a good idea. It's a nontrivial amount of work, because >>>>> of the "all the other S3 method code" part. There's the question of >>>>> functions defined outside of packages: presumably they are still S3 >>>>> methods, with no way to suppress that. >>>> >>>> I am not sure this is the right solution: S3 dispatch will still occur >>>> because we first look at foo.bar exports and then in the S3 registry, >>>> afaicr (the "all the other S3 method code" part). >>>> >>>> If we could move to only looking at the registry for dispatch, there >>>> would be no need to declare situations where we should not dispatch on >>>> foo.bar exports. >>>> >> >>> I think that would break a lot of existing scripts. I think the logic >>> should be something like this. >> >>> For each class in the class list: >> >>> Search backwards through the environment chain. If the current location >>> is a package environment or namespace, look only in the registry. If >>> not, look at all functions. >> >>> If that search failed, try the next class. >> >> Yep---that's what I meant. I forgot to write the "if namespace >> semantics applies" part :-) >> >> Best >> -k >> >>> A variation on the test is: If there's a registry in the current >>> location, look there. But I think the registry is not on the search >>> list, so I'm not sure that would work. >> >>> This assumes that we keep separate registries in each package; if we >>> merge them into one big registry, it gets harder. I'm not familiar >>> enough with the code to know which way we do it. >> >>> Duncan Murdoch >> >> ______________________________________________ >> R-devel at r-project.org mailing list >> https://stat.ethz.ch/mailman/listinfo/r-devel > > >