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 spellcheck workflow #138

Merged
merged 22 commits into from
Mar 12, 2024
Merged

Add spellcheck workflow #138

merged 22 commits into from
Mar 12, 2024

Conversation

sjspielman
Copy link
Member

Purpose

This PR adds the spellcheck workflow. The workflow is setup with three triggers which have the following behavior:

Temporary trigger of PRs to main

We should use this during docs development to check typos as we go. Before onboarding contributors, we should disable this trigger and the associated step post-to-pr (needs 🎟️ ).

cron job and manual dispatch

These are the triggers we expect to use when OpenScPCA gets up and running.

First, GHA cron syntax will not let us run every X weeks, but I found what I think it's a fairly reasonable "hack" that I added for this as posted in #84 (comment). We set a start date (I've put in next sprint for this; note that I've tested this putting in today and it seems to work), and then check if the date the action is trying to run on is actually a multiple of 4 weeks later.
Spellchecking is run if either we're not running as a cronjob, or we're at 4 weeks later.

For both cron & manual, we'll want to end up opening an issue and linking in the artifact file. I've created a template for that issue. Currently those steps are uncommented for a test, but I expect to comment them out for now during dev and revive them at launch (again, we need a 🎟️ ).

Issue Addressed

Closes #84

Any comments, concerns, or questions important for reviewers

No specific comments other than, what looks good to keep and/or discard?

@sjspielman sjspielman marked this pull request as draft March 11, 2024 15:48
@sjspielman
Copy link
Member Author

Hm, I would have expected checkout to work with the default token value, but I don't have access to repo admin settings to make sure everything is setup there. @jashapiro ?

https://github.com/AlexsLemonade/OpenScPCA-analysis/actions/runs/8235720594/job/22520486950#step:3:53

Co-authored-by: Joshua Shapiro <josh.shapiro@ccdatalab.org>
@sjspielman
Copy link
Member Author

Confirming that issue opening works, so I'm now going to comment out those parts of the GHA #141 to avoid spam during docs development.

README.md Outdated Show resolved Hide resolved
Copy link

Spellcheck failed!
Here are the errors.

Copy link

Spellcheck failed!
Here are the errors.

@jashapiro
Copy link
Member

Spellcheck failed! Here are the errors.

This is a kind of annoying way to get the errors, since you have to unzip them. Maybe just cat them as a step in the workflow and link to that step if possible?

@jashapiro
Copy link
Member

Spellcheck failed! Here are the errors.

This is a kind of annoying way to get the errors, since you have to unzip them. Maybe just cat them as a step in the workflow and link to that step if possible?

Another idea is just to have the "fail" step cat the error file before dying. That will display them when clicking "details"

@sjspielman
Copy link
Member Author

This is a kind of annoying way to get the errors, since you have to unzip them. Maybe just cat them as a step in the workflow and link to that step if possible?

(I don't really find this annoying, but I understand this is up for debate!).
I'm looking into a nice way to do this, but getting foiled for reasons I don't understand. Any insights or suggestion for different approach?

I added the step (syntax: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#example-of-a-multiline-string)

      - name: Get the contents
        id: get-typos
        run: |
          {
            echo 'typos<<EOF'
            cat spell_check_errors.tsv
            echo EOF
          } >> "$GITHUB_OUTPUT"

And then changed the PR comment step to print this out, but it's not being printed... I changed the cat spell_check_errors.tsv to just echo "stephanie" and had no luck there either which confused me, since I thought the problem might be something with the artifact but I guess not! Have to head out now but will be back later to poke around.

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

I'm sorry I was unclear! I was suggesting something much simpler than what you are trying to do. Just printing the output table within the workflow step that triggers the failure means that you should see it right away when clicking the failure link either in the email or on the PR page.

.github/workflows/spellcheck.yml Outdated Show resolved Hide resolved
.github/workflows/spellcheck.yml Outdated Show resolved Hide resolved
Co-authored-by: Joshua Shapiro <josh.shapiro@ccdatalab.org>
Copy link

Spellcheck failed!
Here are the errors.

@sjspielman sjspielman marked this pull request as ready for review March 12, 2024 13:38
Comment on lines 69 to 77
- name: Post errors to PR if failed
if: ${{ github.event_name == 'pull_request' && steps.spell.outputs.error_count > 0 }}
uses: peter-evans/create-or-update-comment@v4
id: post-to-pr
with:
issue-number: ${{ github.event.pull_request.number }}
body: |
**Spellcheck failed!**
To see the errors, either you can [download this zip file](${{ steps.artifact-upload-step.outputs.artifact-url }}) or click `Details` in the failed action check.
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that this step be removed entirely. The reason is that everybody watching the repo and/or who has commented on the PR will get notifications of these comments, and that is part of what annoys me. The only person who needs to see this is the person who pushed the most recent changes, and they will be notified by the action failure.

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

LGTM, ∂ some small dictionary changes which probably means some other text changes.

OpenScPCA
openscpca
pre
Pre
Copy link
Member

Choose a reason for hiding this comment

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

Probably not needed, but doesn't hurt

.github/components/dictionary.txt Outdated Show resolved Hide resolved
.github/components/dictionary.txt Outdated Show resolved Hide resolved
.github/components/dictionary.txt Outdated Show resolved Hide resolved
.github/components/dictionary.txt Outdated Show resolved Hide resolved
trainings
transphobic
triaged
WisCon
Copy link
Member

Choose a reason for hiding this comment

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

I need to know

Copy link
Member Author

Choose a reason for hiding this comment

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

We apparently use some of their language in CoC

Comment on lines 36 to 39
echo "FIRST_RUN_DATE: ${{ env.FIRST_RUN_DATE }}" >> $GITHUB_STEP_SUMMARY
echo "current_date: $current_date" >> $GITHUB_STEP_SUMMARY
echo "weekdiff: $weekdiff" >> $GITHUB_STEP_SUMMARY
echo "weekindex: $weekindex" >> $GITHUB_STEP_SUMMARY
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need a step summary for this?

reproducibility
ScPCA
socio
Stumptown
Copy link
Member

Choose a reason for hiding this comment

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

I guess I also want to know how this sneaks in. @jaclyn-taroni?

Copy link
Member Author

Choose a reason for hiding this comment

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

Stumptown Syndicate’s Citizen Code of Conduct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm this website isn't loading though... Do we want to swap the link to https://github.com/stumpsyn/policies/blob/master/citizen_code_of_conduct.md ?

Copy link
Member Author

Choose a reason for hiding this comment

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

the Ubuntu and RLadies links changed also it looks like, so I went ahead and made sure all the links work while I was here.

@sjspielman sjspielman merged commit e805a19 into main Mar 12, 2024
3 checks passed
@sjspielman sjspielman deleted the sjspielman/84-spellcheck-workflow branch March 12, 2024 15:36
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.

Add spellcheck workflow
2 participants