Skip to content
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

We need to change our naming convention -> accidental S3 methods #15

Closed
bw4sz opened this issue Dec 5, 2016 · 4 comments
Closed

We need to change our naming convention -> accidental S3 methods #15

bw4sz opened this issue Dec 5, 2016 · 4 comments

Comments

@bw4sz
Copy link
Contributor

bw4sz commented Dec 5, 2016

I think we are accidentally creating S3 export methods (i.e a predict generic class), by mistake. I noticed that.

For example, the function cannot be called, but is exported and documented.

library(alienR) 
> library(alienR)
> predict.bayesreg
Error: object 'predict.bayesreg' not found
> ?predict.bayesreg

works fine.
http://r-pkgs.had.co.nz/man.html

see Documenting classes, generics and methods.
I think what is happening is predict.bayesreg is a predict generic function for a class bayesreg. I do not know how to set a class, or whether output objects automatically have classes? I suggest we just change all functions from . to _ . For example, predict.bayesreg becomes predict_bayesreg

@bw4sz
Copy link
Contributor Author

bw4sz commented Dec 5, 2016

playing around with @exportClass tag in the roxygen docs.

@bw4sz
Copy link
Contributor Author

bw4sz commented Dec 5, 2016

I changed my names, as part of pull request #11

@SteveViss
Copy link
Contributor

SteveViss commented Dec 5, 2016

Hi @bw4sz,

The convention on functions and object names are already written in the README. Have a look at the first and second bullet points in styleguide section:

  • function names are explicit and with CamelCased verbs, e.g. SimulatesNicheModel.
  • objects are declared with lowercase, underscores and small caps, e.g. trophic_level

I'll change tomorrow all the names already declared in the package according to these.
Cheers,
Steve

@bw4sz
Copy link
Contributor Author

bw4sz commented Jan 3, 2017

Looks like steve took care of this.

@bw4sz bw4sz closed this as completed Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants