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

Add usage to readme and test image #5

Merged
merged 8 commits into from
Mar 8, 2024
Merged

Conversation

jashapiro
Copy link
Member

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)

Copy link
Member

@sjspielman sjspielman left a 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
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

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.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, but it's b/c of a bug:

if (length(arguments) > 1) {

Should be >0, not >1.

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.
Copy link
Member

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.

Copy link
Member Author

@jashapiro jashapiro Mar 8, 2024

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

Copy link
Member

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..

Copy link
Member

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!

@jashapiro jashapiro marked this pull request as ready for review March 8, 2024 15:10
Copy link
Member

@sjspielman sjspielman left a comment

Choose a reason for hiding this comment

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

🚀

@jashapiro jashapiro merged commit bc94922 into main Mar 8, 2024
1 check passed
@jashapiro jashapiro deleted the jashapiro/dockerize-action branch March 8, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants