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 check-spelling action #1222

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add check-spelling action #1222

wants to merge 2 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Apr 24, 2020

This is a proposal.

It adds check-spelling which is a GitHub Action to look at commits and comment on them when it finds things it doesn't recognize as words.

  • As you appear to use a squash-merge strategy, the spell-check commits should generally not appear in your commit history. You can see this action in use by microsoft/terminal.

  • The advice.txt file is (or if you have an advice/ directory with multiple .txt files, the files are) included in the comment. It's intended that the project adjust this file to suit its needs.

  • Some of the whitelist/ files would probably be better suited in dictionary/, but doing that requires including a dictionary into your repository (or me having some guarantees about how to ask for the default dictionary, which I haven't made yet).

  • You don't need to split whitelist.txt into a directory, it could be a single file. I think it's helpful if you're going to eventually move to the dictionary model (as microsoft/terminal does).

  • The distinction between dictionary and whitelist is that the spell checker will recommend removing stale whitelist elements, whereas items in a dictionary are "if this is here, it's ok, if it isn't present, that's also ok".

  • If you have multiple repositories, you could have a common repository with a common dictionary (or even excludes / patterns) and pull that in. This would enable you to reduce duplication.

    • I've prototyped this, but haven't actually implemented it yet. If you're interested, I'd be more than happy to help with this once you get to the point of using this for a second repository. But that's counting my 🐔 before they 🐣.
  • excludes.txt lets you exclude files from scanning, e.g., if you don't care about spelling in pom.xml you could add (?:^|/)pom\.xml$ to the list.

  • domain.txt is some items that I'd love to clean up from this project, but it requires a discussion (I could open an issue). I'm happy to remove/change the comment.

  • Initially, anyone who has a PR from before this is merged will result in things being merged w/o checking, it's possible to catch this by using a schedule.

Note:

  • It's possible to use on: pull_request: -- but that means that people have to create a pull request in order to be told that they should review their changes. That's why I'm using on: push: instead.

  • Also, while I try to encourage projects to avoid spelling errors, I really don't want people to complain about being forced to pass a spell check pass before merging. (Especially if they have a deadline.)

As this action is still under development, I'll look in regularly to help make sure adoption isn't too frustrating (just as you want people to be happy users of your software, so I want you to be happy users of my software -- plus, it helps me identify bugs / areas where I need to improve).

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 24, 2020

I have.

@bbaldino
Copy link
Member

Thanks, this is interesting. Do you have an example PR you could link which shows what it looks like when a spelling error is caught? I know you linked to the terminal repo, but wondering if you have a PR you could link to directly somewhere.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 24, 2020

That's a good point, I should have a good showcase. Here's a simple example:

In microsoft/terminal#5485, github-actions commented and left annotations.

Fwiw, the doubled annotation in line 40 is because the bot is actually reporting the precise character range for each incident (the token is present twice in the line).

  • I could have it suppress that -- so far people haven't complained about that. -- I do take feedback and am actively working to improve the its behavior based on consumers.

For the time being, you can see the logs -- note that GitHub action logs expire after 90 days.

@bbaldino
Copy link
Member

That's a good point, I should have a good showcase. Here's a simple example:

In microsoft/terminal#5485, github-actions commented and left annotations.

Fwiw, the doubled annotation in line 40 is because the bot is actually reporting the precise character range for each incident (the token is present twice in the line).

  • I could have it suppress that -- so far people haven't complained about that. -- I do take feedback and am actively working to improve the its behavior based on consumers.

For the time being, you can see the logs -- note that GitHub action logs expire after 90 days.

Thanks, that's helpful to get a feel for it. I know very little of github actions, so sorry if this is a dumb question: the code under To accept these changes, run the following commands, is there a cleaner/more automated way to do that? I'm imagining clicking something and the action itself being able to push a commit (I thought that's what you were implying when you said "As you appear to use a squash-merge strategy, the spell-check commits should generally not appear in your commit history").

@jsoref
Copy link
Contributor Author

jsoref commented Apr 24, 2020

I've been thinking about it. I definitely intend to improve that part of the flow. Probably sooner rather than later as it's one of the things people have been asking about.

There are a couple of strategies, and I haven't experimented with them yet.

It might be possible for the bot to create a suggestion against a file that isn't in the files being changed.

It's definitely possible for the bot to at least create an "artifact" that one could download and either run or stash into the repository.

I'm pretty sure the bot could be spoken to and then create a commit directly. -- But that's an area of github actions I haven't looked into at all yet.

Worst case, the bot could technically create a PR against the current PR with the changes it's proposing for the whitelist.

I'll hopefully be able to experiment w/ something in this area w/in the next month or so.

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.

3 participants