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

Suggest: no use of attach in examples #10

Closed
richelbilderbeek opened this issue Apr 14, 2021 · 4 comments
Closed

Suggest: no use of attach in examples #10

richelbilderbeek opened this issue Apr 14, 2021 · 4 comments

Comments

@richelbilderbeek
Copy link

Dear SKAT authors, thanks for writing this package! I do have a minor suggestion and I do volunteer to submit a Pull Request , which I describe below.

When viewing the SKAT::SKAT doc ...

library(SKAT)
?SKAT

we find this example code:

data(SKAT.example)
attach(SKAT.example)
# [...]
obj<-SKAT_Null_Model(y.c ~ 1, out_type="C")

I suggest to avoid the use of attach in this example, as lintr, the Tidyverse coding standard, states that attach is an undesirable function.

The easy solution would be to write the same code as such:

data(SKAT.example)
# [...]
obj <- SKAT_Null_Model(data = SKAT.example, formula = y.c ~ 1, out_type = "C")

In this way, the examples depend less on the state of env.

Could you agree that following the most used R coding standard is a good idea? Again, I do volunteer to do so :-)

@richelbilderbeek
Copy link
Author

Contacted the SKAT maintainer by email:

Dear SKAT maintainer,

I contact you as I have posted two Issues at the SKAT GitHub repo and am unsure if you have read these.

If you have and just haven't had the time yet, I would understand :-)

Looking forward to a response one day and cheers, Richel Bilderbeek

@leeshawn
Copy link
Member

Thanks for the suggestion. I modified the example

@leeshawn
Copy link
Member

BTW, adding column name isn't done yet. It will be great if you can do this.

@richelbilderbeek
Copy link
Author

Fixed with 4d12a26, thanks @leeshawn !

richelbilderbeek pushed a commit to richelbilderbeek/reports that referenced this issue Apr 28, 2021
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