Conversation
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Thank you! The fixes look good, so I've merged them separately from your PR. Adding a spell checker as an Action, however, warrants some discussion first.
| - "**" | ||
| schedule: | ||
| # * is a special character in YAML so you have to quote this string | ||
| - cron: '15 * * * *' |
There was a problem hiding this comment.
How would this workflow benefit from being scheduled in addition to running on push?
There was a problem hiding this comment.
Push only works if people enable actions in their forks. For security reasons, this isn't a GitHub default.
The normal approach would be to use the pull_request event, but that event doesn't have comment permissions.
There's a newer event, but I can't recall if I've tested it, and the released action doesn't support that.
| - uses: actions/checkout@v2.0.0 | ||
| with: | ||
| fetch-depth: 5 | ||
| - uses: check-spelling/check-spelling@0.0.16-alpha |
There was a problem hiding this comment.
I'm not sure if we want to add such as early-phase Action as a dependency to our project. Furthermore, I do not think that this warrants being its own workflow, since we already have a lint workflow.
Ideally, a spell checker for our Go source code would be added through our linters. Currently, we use golangci-lint as a linter, and there exists a "misspell" linter that we could enable. Unfortunately, it looks like it only catches misspellings in code comments or strings, but not identifiers.
There was a problem hiding this comment.
microsoft/terminal, microsoft/PowerToys, microsoft/winget-cli to name a few repos that rely upon this action. They all like it except when they realize that their spelling is not great 😄
There was a problem hiding this comment.
I'm not attached to how you anchor/use this action. I'm applying it by using a template. You should be able to place it elsewhere as fits your workflow.
If you run into problems, I'm happy to try to address them in the action.
|
Thanks for merging the spelling fixes. One note on this PR: if you choose to take it, you'll want to ensure that the trunk branch is happy, otherwise people who fork from the unhappy point will be confused & unhappy as the action will complain about the pre-existing typos. Also note that the action's schedule will actually check pulls that don't yet have the action (it uses itself as a baseline if the branch is missing the checker). |
|
@jsoref @alannt777 Thanks for the additional context! After some more deliberation and looking at the implementation of I would be open to adding a linter for misspelled Go identifiers, but it doesn't look like such a linter is readily available. |
2 participants
closes #2442