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

fix(R linting): try installing the R package before linting R language #1911

Merged
merged 8 commits into from
Oct 5, 2021
Merged

fix(R linting): try installing the R package before linting R language #1911

merged 8 commits into from
Oct 5, 2021

Conversation

kpagacz
Copy link
Contributor

@kpagacz kpagacz commented Aug 31, 2021

  • the tool used to lint the R language gives false positives for files inside an R library, which is not installed
  • this change tries to naively install the package in the linted directory

Fixes #1910

I am not particularly experienced in Bash and this is my first PR to this repository, so I would welcome any guidance regarding styling, direction etc.

Proposed Changes

  1. Given the R language files to lint, try to install the R package from the working directory.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

* the tool used to lint the R language gives false positives for files inside an R library, which is not installed
* this change tries to naively install the package in the linted directory

Resolves #1910
Copy link
Collaborator

@admiralAwkbar admiralAwkbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpagacz
Sorry for such a delay on this. Have not had a lot of time lately and have been in maint mode.
Let me know if this looks good to you :)

thanks!

warn "ERROR! Failed to run:[R CMD build] at location:[${WORKSPACE_PATH}]"
else
# Get the build package
BUILD_PKG=$(echo *.tar.gz)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpagacz
I'm not 100% sure how this command is working, but the other 2 make sense :)

So we do a quick check to see if we are validating R files, and that we do have R files to validate, then we run this block of code.

I set it to warn and not error as I'm guessing some people don't have build packages in their repo, and that would cause failures to install something that isn't there...

thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't find the Additional Installs section when I was looking through the code. Thanks for moving it here and all the improvements. My inexperience in bash is certainly showing here.

I agree this line is the weakest link. R CMD build outputs a .tar.gz file with the built package but does not give any options to customize the target path. I can try to regex the line with .tar.gz from the standard output of R CMD build (because the output from the command contains a line with the target file name) and extract the file name that way. I am unsure what's the safest way to proceed here as well.

Do you have a preference?

@admiralAwkbar admiralAwkbar self-assigned this Sep 28, 2021
@admiralAwkbar admiralAwkbar added the enhancement New feature or request label Sep 28, 2021
@admiralAwkbar
Copy link
Collaborator

@kpagacz If you can give me some example code to run the r build command on, I should be able to get the last piece of this done :)
Just not familiar with the language, but if I had access to some test code I can write the last bash :)

@kpagacz
Copy link
Contributor Author

kpagacz commented Oct 4, 2021

Thank you very much for taking this over.

I created an empty R repository: https://github.com/kpagacz/r-test-package

You can clone it and then run the command R CMD build r-test-pacakge and R CMD INSTALL <path to the result of the first command>. Run the commands in the directory containing the r-test-package directory (given directory structure /path/to/repo/here/r-test-package run the commands in the 'here' directory).

@admiralAwkbar
Copy link
Collaborator

@kpagacz Thanks for the test code! Thats all I needed to get this right :)

SHould be able to merge once tests pass

@admiralAwkbar admiralAwkbar merged commit a2193cb into super-linter:main Oct 5, 2021
@kpagacz kpagacz deleted the install_pkg_before_linting branch October 6, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

R linter run by super-linter gives false positives
2 participants