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

Run namespace_linter in a subprocess? Optionally? #912

Open
MichaelChirico opened this issue Mar 4, 2022 · 5 comments
Open

Run namespace_linter in a subprocess? Optionally? #912

MichaelChirico opened this issue Mar 4, 2022 · 5 comments

Comments

@MichaelChirico
Copy link
Collaborator

Note that using check_exports=TRUE or check_nonexports=TRUE will load packages used in user code so it could potentially change the global state.

We could avoid this by running the linter in a subprocess, right? Is it worthwhile to do so?

@renkun-ken
Copy link
Collaborator

Is there a standard that how far we could go to change the global state when linting is run? Looks like there could different levels:

  1. No global state should be changed.
  2. No packages should be loaded except lintr and its dependencies (e.g. xml2, xmlparsedata) itself.
  3. Only known packages used in linters are allowed to be loaded, but not attached, e.g. cyclocomp used in cyclocomp_linter.
  4. Unknown packages used in user code are allowed to be loaded, but not attached, e.g. namespace_linter.

@AshesITR
Copy link
Collaborator

AshesITR commented Mar 5, 2022

I think if we want to support something like this (which would be nice), we should go and run the entire lint() in a separate process instead of having individual linters spawn child processes.

NB though that checkUsage() sometimes reports false positives if code uses variables declared in other locations (so sometimes I tend to run devtools::load_all() before linting to prevent those false positives)

@MichaelChirico
Copy link
Collaborator Author

That makes sense to me -- maybe we can just add an option to lint(): execute_in_subprocess=FALSE by default. Hopefully there won't be too much overhead to maintain a branch like this.

@AshesITR
Copy link
Collaborator

AshesITR commented Mar 7, 2022

Maybe we can make use of callr::r() for this.
And we should make sure that lint_dir() and lint_package() handle this as well without spawning one process per file.

@AshesITR
Copy link
Collaborator

Crossref #240; also interesting for parallelization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants