-
Notifications
You must be signed in to change notification settings - Fork 421
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
add lintr step to PR workflow #3072
Conversation
9175ef8
to
44b85f5
Compare
44b85f5
to
be3c42c
Compare
Thanks @nsoranzo . Looks quite good to me now. Caching of the R packages seems not to work yet. Any idea where the annotation:
comes from? |
ced503f
to
8a37e79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggested changes to print the full relative path of the files with linting errors.
Nice! You could now check if fixing an R script makes the linter step pass correctly. |
7d0948c
to
5962f1b
Compare
Seems to work -- also the caching runs if the lint step is successful. I think the last question is if the caching of the R packages is fine as it is. Maybe it should be versioned? |
Co-authored-by: Nicola Soranzo <[email protected]>
5962f1b
to
41b17c3
Compare
Co-authored-by: Nicola Soranzo <[email protected]>
OK. Cool. Not sure about R package management. What happens if a new version of lintr or one of its requirements is available? |
As it is now, the present version of lintr will be used from the cache until we change the R version in the matrix. I don't think it is a big deal, we can change that later if needed. |
also integrate latest changes (from galaxyproject/tools-devteam#560) from devteam repo.
FOR CONTRIBUTOR: