Workflow setup #5
Conversation
|
Thanks @JosephCrispell will have a look later today! |
There was a problem hiding this comment.
Hi @JosephCrispell , I've made a start at looking through this. It all looks good technically, but I unfortunately cannot get this to work on my ONS machine. I believe this is related again to the issues I had setting up precommit at all on the ONS machine, and issues to do with renv and attempted package retrievals when trying to run precommits. I will try to test on my personal machine, to see if I can separate to what extent this is a single-setup issue or a wider ONS network issue. Apologies for the delay!
| saveWidget(timevis(gantt, groups=groups, options=options, width="100%", showZoom=TRUE), | ||
| file, selfcontained=FALSE) # Unfortunately self-contained doesn't work | ||
| saveWidget(timevis(gantt, | ||
| groups = groups, options = options, | ||
| width = "100%", showZoom = TRUE | ||
| ), | ||
| file, | ||
| selfcontained = FALSE | ||
| ) # Unfortunately self-contained doesn't work |
There was a problem hiding this comment.
Just a comment and not an issue - this is interesting, was this an automatic edit? Stylewise I agree with most of it, apart from perhaps the lack of an indent on L47-48 (it's part of the saveWidget() call?)
There was a problem hiding this comment.
Agreed, that's styler working it's magic - going to go back in and add some indents now. Good catch! 🎣
| - [Rmarkdown cheatsheet](https://rstudio.com/wp-content/uploads/2015/02/rmarkdown-cheatsheet.pdf) | ||
| - [Interactive plotting with plotly](https://plotly.com/r/) | ||
| - [Programming in R](https://www.tutorialspoint.com/r/index.htm) | ||
| - [Troubleshooting with stackoverflow](https://stackoverflow.com/questions/tagged/r) |
There was a problem hiding this comment.
Again a comment unrelated to the precommit workflow per se, am I right in thinking this file is pretty much the same as DashboardDemos/InteractiveDashboard_01-09-20.Rmd? I.e. also using flexdashboard as opposed the the traditional Shiny layout?
There was a problem hiding this comment.
Yup, it's flexdashboard and shiny. I haven't used the traditional Shiny layouts before - it would be good to edit this script to not rely on flexdashboard - Added as an issue just now 👍
| hooks: | ||
| - id: style-files | ||
| args: [--style_pkg=styler, --style_fun=tidyverse_style] | ||
| - id: spell-check |
There was a problem hiding this comment.
I found that in my (working, personal) set up, the precommit spell checker and secrets checker fails when it needs to check (knitted) html and docx files. Does these pass for you @JosephCrispell? The only way to "make" precommit pass in my case was to add explicit exclusions for (knitted) .html and .docx files. I will push some edits to the branch to show what I did.
I think including .html and .docx in this case as explicit exclusions is OK, because (1) we probably don't need extra spellchecks by precommit on docs that were knitted from an Rmd, which would have already been checked for errors - same with knitted htmls. Similarly, any stray secrets in a knitted html would have been present in the source Rmd?
There was a problem hiding this comment.
Completely agreed, doesn't make sense to spell check Rmd outputs alongside original scripts so thanks for editing the exclusions - not quite sure why this didn't throw any errors when I was running 🤔
There was a problem hiding this comment.
Hi @JosephCrispell This all looks great! I had to test this on my personal laptop as my precommit set up on the ONS laptop still appears to be broken (it would be great to get someone else to check with an ONS machine, just to make sure it isn't just me!).
My only suggestion on the precommit workflow is to add explicit exclusions to the spellchecker and secrets checker for html and docx files - in my case, precommit would fail on knitted HTML files. I don't know why, but do we need spell- and secret checks for HTML and docx files when these have been knitted from Rmd's? I can push the suggested changes to this branch, if that's helpful? (I'm unsure whether this merge request is closed when I submit the review?)
Suggested exclusions for .html and .docx files for spell and secrets checker - they failed on newly knitted files in my precommit set up.
|
@JosephCrispell have a look at e30ad3c, we can discuss on Monday |
|
Hi @jeroenminderman, Thanks so much for reviewing the codebase and checking out the new workflow. I am really pleased with it and it was easy to set up (although painful to get passed all the hooks). In return, I'm going to test getting it to work on my ONS laptop and then get back to you - it would be really useful to get this sorted and then feed our learning into the GitHub training materials. Thanks again! Joe |
|
Thanks @JosephCrispell and no problem at all. It's a great repo this and I'm hoping to use material from this in some training for NISR (which would be useful to discuss at some point, separately). |
|
Hi @jeroenminderman, I think we can merge this pull request and I've created an issue for testing on ONS infrastructure where we can continue the conversation - outputs of this will be more directly relevant to the ONS GitHub guidance than the resources in this repo. Thanks again for taking a look at the code! |
Description
Adding in a R based precommit workflow with some additional generic hooks. This has involved updating scripts to adhere to styler and lintr.
Changes
.pre-commit-config.yamlprecommit workflow.lintr) with some bespoke settingsInstallation steps:
pip3 install pre-commitinstall.packages("precommit")precommit::use_precommit()