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

expect_lint() should be exported #178

Closed
fangly opened this issue Oct 20, 2016 · 9 comments
Closed

expect_lint() should be exported #178

fangly opened this issue Oct 20, 2016 · 9 comments

Comments

@fangly
Copy link
Contributor

fangly commented Oct 20, 2016

Hi,

I am writing some custom linters following the document at https://cran.r-project.org/web/packages/lintr/vignettes/creating_linters.html . It turns out to be impossible to add testthat tests because the "expect_lint()" expectation is not exported by lintr.

I am thus forced to use the lintr:::expect_lint hack at the moment.

Could you please add a small "#' @export" statement in "R/expect_lint.R" please?

Cheers,

Florent

@jimhester
Copy link
Member

Sure I could do it, but since you seem to know what to do why not make a pull request instead? See http://r-pkgs.had.co.nz/git.html#git-pullreq and https://help.github.com/articles/creating-a-pull-request/ for information on how to create one.

Because this change requires re-documenting don't forget to run devtools::document() or roxygen2::roxygenize() on the package and commit the NAMESPACE changes as well.

@fangly
Copy link
Contributor Author

fangly commented Oct 21, 2016

Unfortunately, I am behind an institutional proxy/firewall that makes cloning/pushing github repos impossible.
Cheers,
Florent

@gaborcsardi
Copy link
Member

@fangly For this simple change, you can just edit the file(s) online.

@fangly
Copy link
Contributor Author

fangly commented Oct 21, 2016

Thanks gaborcsardi, but I would need to make a fork first. And then I wouldn't be able to run devtools::document() anyway.

@fangly
Copy link
Contributor Author

fangly commented Oct 21, 2016

To follow up on this issue, the tutorial I mentioned above also list the "ids_with_token" function, which is defined in R/utils.R, but not exported.
Again, it might be useful to export this function so that the tutorial can be completed. Cheers!

@jimhester
Copy link
Member

The vignette assumes you will be adding new linters to the lintr package, not creating them in external packages, which is why it references internal functions.

As far as not being able to push you could also do the commits locally then use git format-patch to create the patch and post it here.

@fangly
Copy link
Contributor Author

fangly commented Oct 24, 2016

Well, I suppose you need to decide then:
1/ Either update the tutorial and clarify that this is intended for contributing linters to lintr (and then the first described step should be to fork). Then you can keep "expect_lint" and "ids_with_token" private.
2/ Or export "expect_lint" and "ids_with_token" and make sure that the tutorial works for people writing a custom linter on their own.

Considering that not everyone may be able to contribute the linter they wrote, and you may not want to accept the linters written by everyone, I would be in favor of option 2.

Cheers,

Florent

@russHyde
Copy link
Collaborator

expect_lint is now @exported in master. Suggest closing

@jimhester
Copy link
Member

@russHyde Feel free to close them yourself!

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

4 participants