-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S3 methods not using arguments consistent with their generic #117
Comments
Okay, I think that makes sense, and it looks like it's not going to mess things up. I'm in the middle of using Bacon and doing some other stuff, but I'll fix that tonight. |
Yeah, I think if you change the generics to match the first argument of the method then you shouldn't have to change much if anything in the method function body other than to add The only thing to think about is whether you need this method at all. Are you intending a |
Yes, the idea is that if you use Sorry, so I don't want just |
Well no, it doesn't matter what argument they go in as, but it would make for odd documentation if you have to write: siteid: either a character string containing the required site ID, or an object of class It would be easier to just call this first argument |
Okay, that's perfect. That's why I had the |
Okay, I seem to be having some problems here. The issue seems to be that if I use the model:
The S3 warning goes away but I get a Is there a soultion to this? Is it okay to ignore the |
No, you can't ignore it. The problem is that you aren't documenting the S3 methods for the generic in the same Rd file. At the moment you are generating an The way I do this is:
If the methods use the same arguments as each other, then it will be simpler to document them in the roxygen block for the generic; you don't want to repeat yourself for each method and you probably don't want them all spread out either if the same argument applies to several methods. |
Okay, I get this, that's what I've been trying to do, but then when I do:
I only get the help for the generic, right? I'd rather see the generic help file have all the help that the |
Okay, I think I've figured this out. I just need more complete documentation in my files. Working on it. Thanks @gavinsimpson, sorry for the bother. |
Finished documenting and rewriting help sections for |
@SimonGoring Has all this fixing of the S3 methods just broken the paper? marion <- get_site(sitename = 'Marion Lake%') fails now because the first argument name is |
Ugh, yes. Is it possible to add |
Okay, I've fixed this in commit 3eeeb18, by sticking sitename back in and then adding some checks around |
Uggh, that's nasty. Why not have
Then get rid of |
Yes, I think that's what I've got to do. I tried to fix it, but it's just too gross to mess around with, |
@SimonGoring Do you want me to fix it back along the lines I suggested? |
I have the fix done and I'm going to push it (it checks fine locally), but On Wed, Feb 25, 2015 at 11:10 AM, Gavin Simpson notifications@github.com
|
😢 One feature/fix at a time :-) Atomise your commits/pushes otherwise you'll keep on having trouble with merge conflicts or other contributors will hold off doing work as they won't know what features of the codebase you may or may not have worked on ;-) |
I know. I'm still working on building the habits necessary for good On Wed, Feb 25, 2015 at 11:24 AM, Gavin Simpson notifications@github.com
|
👍 💯 |
Is there an emoji for hanging your head in shame? On Wed, Feb 25, 2015 at 12:00 PM, Karthik Ram notifications@github.com
|
This is (finally) fixed in commit 32ba77b |
@SimonGoring I don't know why
You have #L45 get_site <- function(sitename, altmin, altmax, loc, gpid, ...) but only #57 get_site.default <- function(sitename, ...) This is in contravention of the above rule. The only surprising thing is that You either need to have all the methods have at a minimum the following arguments: get_site.foo <- function(sitename, altmin, altmax, loc, gpid, ...) or just have the generic be get_site <- function(sitename, ...) and put the extra arguments only in the definitions of the methods that need or can work with those argument. |
I believe it's a good practice to leave the parameter of a generic function as ... |
When you do
R CMD check
on the neotoma tarball lots of warnings ar generated along the lines of:This is because the S3 methods have been incorrectly set up. An S3 method must have the same arguments as the generic. A method can have additional arguments but it must have the arguments of the generic. In the case above, the
get_dataset.default
method has first argumentsiteid
where as the generic has argumentx
. The method is also lacking the...
argument.The obvious fix is to rewrite the definition of the generic as:
and the method as
This needs doing for all (or most) of the new S3 methods added recently.
The text was updated successfully, but these errors were encountered: