-
Notifications
You must be signed in to change notification settings - Fork 1
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 usage to readme and test image #5
Conversation
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.
Made a test repo here: https://github.com/sjspielman/test-spellcheck
Workflow is working as expected, and I added a feature which I left a comment about in review:
https://github.com/sjspielman/test-spellcheck/blob/main/.github/workflows/spellcheck.yml
uses: actions/checkout@v4 | ||
|
||
- name: Spell check action | ||
uses: alexslemonade/spellcheck |
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.
As is this does not work because it needs a tag, but it does work with alexslemonade/spellcheck@main
.
Easy enough to pop in when we decide on the tag, which we can do in this PR or after.
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.
I made some fixes, so I think the move is going to be to merge this PR, tag a release, then update a couple things in a new PR.
README.md
Outdated
dictionary: components/dictionary.txt | ||
|
||
- name: Upload spell check errors | ||
uses: actions/upload-artifact@v2 |
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.
If we bump this to v4 (and add some write
permissions), we can have nice things:
In the context of OpenScPCA-analysis, we'd post the artifact link to an issue.
In the context of other repos where we do have a PR trigger, we'd post as a PR comment.
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.
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.
updated
Note that the `dictionary` input to the spell check step is optional and defaults to `components/dictionary.txt`. | ||
If you want to use a different dictionary, you can specify the path to the dictionary file in your repository. | ||
|
||
You can also specify specific files to spell check using the `files` input to the `alexslemonade/spellcheck` step. |
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.
This did not work as expected: https://github.com/sjspielman/test-spellcheck/pull/2
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.
Note that the `dictionary` input to the spell check step is optional and defaults to `components/dictionary.txt`. | ||
If you want to use a different dictionary, you can specify the path to the dictionary file in your repository. | ||
|
||
You can also specify specific files to spell check using the `files` input to the `alexslemonade/spellcheck` step. |
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.
But if you pass in files
, you have to pass in dictionary
. Otherwise the script as written will interpret the files
argument as the dictionary file.
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.
I would expect this to be handled by the fact that the arguments are named in the context of the action. So inputs.dictionary
should get a default value if it is not provided. Was that not the case?
I added a fix in case an empty string is given, but if you use
with:
files: test-file.md
I would expect the dictionary
to be populated with the default value
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.
So inputs.dictionary should get a default value if it is not provided. Was that not the case?
I did not test this actually - this is just my sense of looking at the R code directly. Let me check formally..
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.
I would expect the dictionary to be populated with the default value
Confirmed, as expected!
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.
🚀
This PR currently tests the workflow in the
test.yaml
action, but the main purpose is to test that the action works as expected somewhere else following the instructions in the readme that I have added.@sjspielman: Can you try adding that workflow to another repo to see if it works as expected?
If it does, we can decide on a release version, add those tags as appropriate here, then merge and tag the actual release. (Probably removing the test action, or making it only run on workflow_dispatch)