Make it a full action #2
Conversation
There was a problem hiding this comment.
what do you think about optparse?
I was also thinking about whether we want a second script for checking jupyter notebooks. I had thought pyspelling might be an option for us, but I was tricked into thinking it would be able to check jupyter notebooks just because it's written in python :/ . Looks like someone tried to make it happen once, but this doesn't seem like something we also want to make happen. This remains the only real tool I can find, and it's interactive:https://github.com/jupyterlab-contrib/spellchecker . So, I don't really have anything to suggest to automate spell checking there, meaning what we've got is what we've got!
| @@ -3,3 +3,4 @@ | |||
| This repository contains a Docker image with the [R `spelling` package](https://cran.r-project.org/web/packages/spelling/index.html) installed. | |||
| The role of this repository is to facilitate spell checking actions across `AlexsLemonade` repositories. | |||
|
|
|||
| speeling | |||
There was a problem hiding this comment.
I think the important question here is whether we want to just have this here, or integrate it more sneakily. Like,
The role of this repository is to facilitate speell checking actions across
AlexsLemonaderepositories.
Either way, it must stay in some capacity.
| image: 'Dockerfile' | ||
| args: | ||
| - ${{ inputs.dictionary }} | ||
| - ${{ inputs.files }} |
| # dictionary is required first argument | ||
| dict_file <- arguments[1] | ||
|
|
||
| arguments <- arguments[-1] |
There was a problem hiding this comment.
I kind of wonder if we want to just do optparse here. It'll add another dependency but maybe it's worth it to be more explicit, especially if we also want to allow for another argument with a different pattern besides Rmd/rmd/md
There was a problem hiding this comment.
We could, but I think for the first version this probably makes the most sense.
spell-check.R
Outdated
| # Save spell errors to file | ||
| readr::write_tsv(spelling_errors, "spell_check_errors.tsv") | ||
| # Save spelling errors to file | ||
| write.table(spelling_errors, "spell_check_errors.tsv", sep = "\t", quote = FALSE, row.names = FALSE) |
There was a problem hiding this comment.
I mean, we could install readr too. I'm not actually opposed to that, given the ugly state of affairs there.
There was a problem hiding this comment.
I don't have much preference either way, so up to you
| # Save error count to GITHUB_OUTPUT | ||
| system(paste0("echo 'error_count=", nrow(spelling_errors), "'>> $GITHUB_OUTPUT")) |
File an issue for it! One idea I had was to export the notebook as markdown using |
| - name: Spell check action | ||
| uses: ./ # Uses an action in the root directory |
There was a problem hiding this comment.
You have this as a TBD in your opening PR comment, but I don't think it's TBD here - it's TBD for porting this action into other repos
There was a problem hiding this comment.
I don't know what you mean. It is a change that needs to be made in action.yaml, but can't be made until we have at least one build pushed. It can happen in this PR or a later one. (There is an interesting cyclical dependency, which would be removed be deleting the test action and replacing it with example implementation in the readme)
There was a problem hiding this comment.
Right - I am just noting that the TBD phrasing you have in your opening comment is relevant to next steps, but not to merging this PR.
There was a problem hiding this comment.
Either way. I could implement it all here, or in a next PR.
There was a problem hiding this comment.
I'd probably rather have this all merged in so we can test with a built image.
There was a problem hiding this comment.
I'd probably rather have this all merged in so we can test with a built image.
So, implement it all here? that's fine with me
There was a problem hiding this comment.
Sorry, I meant merge in the current state. That will trigger a build and push of the image, so we can then test with a pulled image in the next update.
There was a problem hiding this comment.
It needs to be this version of the Docker image/script, with the changes to inputs and outputs.
Here I am adding
action.yamlto make this a fully operational github action. I also modified the script to output counts directly to theGITHUB_OUTPUTenvironment variable.There is also a test action to demonstrate usage.
Still TBD: fixing the intentional spelling error and using a docker image from the registry in the action (rather than the Dockerfile) to prevent rebuilding every time the action runs.