RFC: Declaring "foo.bar" as nonS3method() ?!

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Declaring "foo.bar" as nonS3method() ?!

Martin Maechler
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.

Martin Maechler,
ETH Zurich

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Fox, John
And my non-cross-posted cross-posting:

"Dear Martin,

Thank you for addressing this issue. Introducing a nonS3method() directive in NAMESPACE seems a reasonable solution. It could replace export() for functions with "."s in their names.

Best,
 John"

On Fri, 12 Jun 2015 10:12:07 +0200
 Martin Maechler <[hidden email]> 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.
>
> Martin Maechler,
> ETH Zurich
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

------------------------------------------------
John Fox, Professor
McMaster University
Hamilton, Ontario, Canada
http://socserv.mcmaster.ca/jfox/

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Duncan Murdoch-2
In reply to this post by Martin Maechler
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.

Duncan

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Duncan Murdoch-2
In reply to this post by Fox, John
On 12/06/2015 6:41 AM, John Fox wrote:
> And my non-cross-posted cross-posting:
>
> "Dear Martin,
>
> Thank you for addressing this issue. Introducing a nonS3method() directive in NAMESPACE seems a reasonable solution. It could replace export() for functions with "."s in their names.

I think these are separate questions:  all.effects() could be exported
or not, and independently could be an S3 method or not.

Duncan Murdoch

> Best,
>  John"
>
> On Fri, 12 Jun 2015 10:12:07 +0200
>  Martin Maechler <[hidden email]> 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.
>>
>> Martin Maechler,
>> ETH Zurich
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
> ------------------------------------------------
> John Fox, Professor
> McMaster University
> Hamilton, Ontario, Canada
> http://socserv.mcmaster.ca/jfox/
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Kurt Hornik-5
In reply to this post by Duncan Murdoch-2
>>>>> 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.

-k

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Duncan Murdoch-2
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.

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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Kurt Hornik-5
>>>>> 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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

hadley wickham
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 <[hidden email]> 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
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel



--
http://had.co.nz/

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Martin Maechler
>>>>> Hadley Wickham <[hidden email]>
>>>>>     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 <[hidden email]> 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
    >>
    >> ______________________________________________
    >> [hidden email] mailing list
    >> https://stat.ethz.ch/mailman/listinfo/r-devel



    > --
    > http://had.co.nz/

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Duncan Murdoch-2
In reply to this post by hadley wickham
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 <[hidden email]> 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
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Tierney, Luke
The notes available off the devloper page
https://developer.r-project.org/ describe some of the rationale for
the S3 method search design. One thing that has changed since then is
that all packages now have name spaces. We could change the search
algorithm to skip attached package exports (and package imports and
base), which would require methods defined in packages that are to be
accessible outside the package to be declared.  Methods defined inside
a package for internal use or methods defined in scripts not in
packages would still be found. Packages not currently registering
their methods would have to do so -- not sure how many that would
affect. Testing on CRAN/Bioc should show how much of an effect this
would have and whether there are any other issues.

Best,

luke

On Fri, 12 Jun 2015, Duncan Murdoch wrote:

> 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 <[hidden email]> 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
>>>
>>> ______________________________________________
>>> [hidden email] mailing list
>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>>
>>
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

--
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa                  Phone:             319-335-3386
Department of Statistics and        Fax:               319-335-3017
    Actuarial Science
241 Schaeffer Hall                  email:   [hidden email]
Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Henrik Bengtsson-4
Analogously to how S4 methods are declared in the code, cf.
methods::setMethod(), I'd find it more natural to also declare S3
methods in the code and note in the NAMESPACE.  For example:

# S3 method summary() for class 'aov':

summary.aov <- function(x, ...) {
  # something
}
S3class(summary.aov) <- "aov"

with

`S3class<-` <- function(x, value) {
  attr(x, "S3class") <- value
  x
}

For backward compatibility, if 'S3class' is not set, the default
could/should be to infer it using the current strategy, i.e. the part
after the last period/dot plus other bells'n'whistles discussed.  If
all S3 methods had attribute 'S3class' set, there would be no need to
declare the non-S3 case.

Finally, to explicitly declare a function _not_ to be a S3 method, one
could allow for

S3class(all.effects) <- FALSE


At this point, I need  to bring up the wish of have a core R function,
again cf. setMethod(), doing the above for us, e.g.

setMethodS3("summary", "aov", function(x, ...) {
  # something
})

It can be extremely light weight and would resemble what setMethod()
does for S4 methods.

Also, with the 'S3class' attribute set, one could imagine not having
to declare them as S3method("summary", "aov") in the NAMESPACE.  This
could be fully automatic (and backward compatible for migration).
Absolutely not a rant, but from a developers point of view I always
found it a bit ad hoc to have to declare S3 methods in the NAMESPACE
rather than in the code.  We're not doing it for S4 methods, so why
for S3 ones? (BTW, I think I understand why).  For the same reason,
I'd would think adding NAMESPACE declaration nonS3method() would just
add another workaround.

The above would be backward compatible, allow for a long-term
migration, while allowing folks to use periods/dots however they wish.
It would also allow code inspections such as 'R CMD check --as-cran'
to avoid false positives.

/Henrik


On Fri, Jun 12, 2015 at 8:30 AM,  <[hidden email]> wrote:

> The notes available off the devloper page
> https://developer.r-project.org/ describe some of the rationale for
> the S3 method search design. One thing that has changed since then is
> that all packages now have name spaces. We could change the search
> algorithm to skip attached package exports (and package imports and
> base), which would require methods defined in packages that are to be
> accessible outside the package to be declared.  Methods defined inside
> a package for internal use or methods defined in scripts not in
> packages would still be found. Packages not currently registering
> their methods would have to do so -- not sure how many that would
> affect. Testing on CRAN/Bioc should show how much of an effect this
> would have and whether there are any other issues.
>
> Best,
>
> luke
>
>
> On Fri, 12 Jun 2015, Duncan Murdoch wrote:
>
>> 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 <[hidden email]>
>>> 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
>>>>
>>>>
>>>> ______________________________________________
>>>> [hidden email] mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>
>>>
>>>
>>>
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>
> --
> Luke Tierney
> Ralph E. Wareham Professor of Mathematical Sciences
> University of Iowa                  Phone:             319-335-3386
> Department of Statistics and        Fax:               319-335-3017
>    Actuarial Science
> 241 Schaeffer Hall                  email:   [hidden email]
> Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu
>
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Martin Maechler
In reply to this post by Tierney, Luke
>>>>> Luke Tierney <[hidden email]>
>>>>>     on Fri, 12 Jun 2015 10:30:29 -0500 writes:

    > The notes available off the devloper page
    > https://developer.r-project.org/ describe some of the rationale for
    > the S3 method search design. One thing that has changed since then is
    > that all packages now have name spaces. We could change the search
    > algorithm to skip attached package exports (and package imports and
    > base), which would require methods defined in packages that are to be
    > accessible outside the package to be declared.  Methods defined inside
    > a package for internal use or methods defined in scripts not in
    > packages would still be found. Packages not currently registering
    > their methods would have to do so -- not sure how many that would
    > affect. Testing on CRAN/Bioc should show how much of an effect this
    > would have and whether there are any other issues.

    > Best,
    > luke

Thanks a lot Luke, for the extra perspective.  
I think the four R core commenters here (Duncan, Kurt, Luke and
me) agree that this is not trivial to implement, but hopefully
not too hard either, and I think we also +- agree that it seems
desirable to try adding a bit more flexibility in how functions
are "made into" S3 methods.

I had not envisaged to change the S3 method search
algorithm but rather to tweak part of it "database" but am aware
that I don't know enough of the details.
Also, I did not find which notes (from developer.r-project.org)
you were refering to.

Given the broad agreement that we want to start working /
investigating this,  we can well close the thread here for now
(and deal with ideas, issues, details within R-core).

Martin

    > On Fri, 12 Jun 2015, Duncan Murdoch wrote:

    >> 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 <[hidden email]> 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
    >>>>
    >>>> ______________________________________________
    >>>> [hidden email] mailing list
    >>>> https://stat.ethz.ch/mailman/listinfo/r-devel
    >>>
    >>>
    >>>
    >>
    >> ______________________________________________
    >> [hidden email] mailing list
    >> https://stat.ethz.ch/mailman/listinfo/r-devel
    >>

    > --
    > Luke Tierney
    > Ralph E. Wareham Professor of Mathematical Sciences
    > University of Iowa                  Phone:             319-335-3386
    > Department of Statistics and        Fax:               319-335-3017
    > Actuarial Science
    > 241 Schaeffer Hall                  email:   [hidden email]
    > Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Declaring "foo.bar" as nonS3method() ?!

Fox, John
Dear Martin et al.,

Thank you for considering so thoroughly the issue that I raised.

Best,
 John

On Sat, 13 Jun 2015 13:35:41 +0200
 Martin Maechler <[hidden email]> wrote:

> >>>>> Luke Tierney <[hidden email]>
> >>>>>     on Fri, 12 Jun 2015 10:30:29 -0500 writes:
>
>     > The notes available off the devloper page
>     > https://developer.r-project.org/ describe some of the rationale for
>     > the S3 method search design. One thing that has changed since then is
>     > that all packages now have name spaces. We could change the search
>     > algorithm to skip attached package exports (and package imports and
>     > base), which would require methods defined in packages that are to be
>     > accessible outside the package to be declared.  Methods defined inside
>     > a package for internal use or methods defined in scripts not in
>     > packages would still be found. Packages not currently registering
>     > their methods would have to do so -- not sure how many that would
>     > affect. Testing on CRAN/Bioc should show how much of an effect this
>     > would have and whether there are any other issues.
>
>     > Best,
>     > luke
>
> Thanks a lot Luke, for the extra perspective.  
> I think the four R core commenters here (Duncan, Kurt, Luke and
> me) agree that this is not trivial to implement, but hopefully
> not too hard either, and I think we also +- agree that it seems
> desirable to try adding a bit more flexibility in how functions
> are "made into" S3 methods.
>
> I had not envisaged to change the S3 method search
> algorithm but rather to tweak part of it "database" but am aware
> that I don't know enough of the details.
> Also, I did not find which notes (from developer.r-project.org)
> you were refering to.
>
> Given the broad agreement that we want to start working /
> investigating this,  we can well close the thread here for now
> (and deal with ideas, issues, details within R-core).
>
> Martin
>
>     > On Fri, 12 Jun 2015, Duncan Murdoch wrote:
>
>     >> 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 <[hidden email]> 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
>     >>>>
>     >>>> ______________________________________________
>     >>>> [hidden email] mailing list
>     >>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>     >>>
>     >>>
>     >>>
>     >>
>     >> ______________________________________________
>     >> [hidden email] mailing list
>     >> https://stat.ethz.ch/mailman/listinfo/r-devel
>     >>
>
>     > --
>     > Luke Tierney
>     > Ralph E. Wareham Professor of Mathematical Sciences
>     > University of Iowa                  Phone:             319-335-3386
>     > Department of Statistics and        Fax:               319-335-3017
>     > Actuarial Science
>     > 241 Schaeffer Hall                  email:   [hidden email]
>     > Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel