Skip to content

Conversation

@JZL
Copy link

@JZL JZL commented Feb 1, 2025

Hi

This is an (almost entirely) cosmetic PR which corrects some miscellaneous spelling and drafts a github workflow to add spell-checking to PR's.

In the first commit, there are only two changes in the code not comments/docs:

but which should only affect those blocks.

In the second commit, I added a github workflow which runs the same commands automatically. However there will almost certainly be false-positives due to how stringently hunspell checks, which the committer would have to manually add to the whitelist. I don't want to annoy people. I'm equally happy to automate periodically running the commands on my own side (or even make a github action which runs on a schedule, opening a new PR if it finds typos).

If we do think adding a workflow is a good idea, it might be best to only run on the files changed within the PR. Otherwise, if one PR/commit adds a typo, it will error on all future PR until it's fixed.

Here's an example on my branch of the bot

What type of PR is this? (check all applicable)

  • Feature
  • Bug Fix
  • Documentation Update
  • Code Refactor
  • Performance Improvements

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and .md files under /docs/src have been updated if necessary.
  • The latest changes on the target branch have been incorporated, so that any conflicts are taken care of before merging. This can be accomplished either by merging in the target branch (e.g. 'git merge develop') or by rebasing on top of the target branch (e.g. 'git rebase develop'). Please do not hesitate to reach out to the GenX development team if you need help with this.
  • Code has been tested to ensure all functionality works as intended.
  • CHANGELOG.md has been updated (if this is a 'notable' change).
  • I consent to the release of this PR's code under the GNU General Public license.

How this can be tested

Only cosmetic doc/comment changes. I will read through one more time to make sure I didn't inadvertently change actual code.

Post-approval checklist for GenX core developers

After the PR is approved

  • Check that the latest changes on the target branch are incorporated, either via merge or rebase
  • Remember to squash and merge if incorporating into develop

JZL added 2 commits January 31, 2025 23:52
Ran
```
typos --words --config=typos.toml | sort -u | hunspell -p hunspell_whitelist -
```
and manually fixed.

`hunspell` is from apt's `hunspell hunspell-en-us` and `typos`
from https://github.com/crate-ci/typos
Runs `hunspell` spell checker on each PR, using `typos` as a tokenizer. It uses
`docs/typos.toml` to ignore certain file types and the `hunspell_whitelist` for
correct words (/tokens) not in the default dictionary.
Copy link
Collaborator

@cfe316 cfe316 left a comment

Choose a reason for hiding this comment

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

This is great. Many of these typos have been bugging me for a long time.
I'm not sure it needs to be run automatically but others should weigh in.

I found only

  • two sets of comments/textual correcting to the wrong words
  • one code false correction (which would break everything 👎 )

@cfe316
Copy link
Collaborator

cfe316 commented Feb 1, 2025

Do please point your PR at the develop branch for changes other than critical bug fixes.

@lbonaldo lbonaldo changed the base branch from main to develop February 5, 2025 00:15
sambuddhac and others added 5 commits May 15, 2025 09:15
Changed to programmatically
Changed lines 34, 35, 36 to planning instead of planing
Changed BffExpr to AffExpr
@JZL
Copy link
Author

JZL commented May 23, 2025

Caught up on the recent develop branch. Thanks Sam for merging and updating! I will look through one more time.

But don't know what the feeling is on it automatically running on PR's. There were some amount of typos in the Allam cycle push, which points towards having it in PR. But, unless the whitelist is maintained, it can quickly get annoying. And I'd want to make it so it only checks files which are changed within-PR so it isn't spamming a PR with unrelated typos. I don't mind setting an every-so-often reminder to rerun it myself and opening a PR, but it could mean more periodic PR's

I was also trying to think of a way to programatically find changes which were in the code not strings to examine much closer, to avoid a future Bff problem. It's tricky b/c knowing if you're within a multi-line docstring depends on the broader file context, but I was working on using semgrep to parse the code. It mostly works

@lbonaldo
Copy link
Collaborator

Thanks so much @JZL for this PR!
Regarding your question, I’d probably be in favor of including this check as a scheduled Gh action (either weekly or within each release cycle, e.g., when we open the release branch) that opens a PR if needed. We can then adjust the whitelist as necessary and merge the PR.
I’d love to run it on every PR as well, though I’m not sure how much effort it would take to keep the whitelist up to date and ensure the PR passes the check. Maybe for now, we can start with the scheduled PR approach and see how it goes.

@JZL
Copy link
Author

JZL commented Jun 16, 2025

Cool I think we're on the same page :-). I can make the scheduled GH PR directly from my commit above. Barring it being too annoying, I have a slight preference for weekly/monthly scheduled because when there are lots of separate typos, it can be pretty time consuming to do. But I'll assign the scheduled PR's to myself!

For the on-PR feedback, as a low priority task, I'd like to see if I can get the CI to give non-blocking feedback on PR's (maybe an automated comment after it runs?) and have it only check the PR's specific changed files (to avoid cascading false positives). If I can figure it out reliably with the diff I might even have it only spellcheck the changed lines. I'll try to make the comment pretty unobtrusive and clear that the author doesn't have to worry about it, but I think it can be helpful feedback. Does that seem good to try?

@lbonaldo
Copy link
Collaborator

The plan sounds great to me (weekly checks work well)! Thanks @JZL. Feel free to assign it to both of us, it shouldn't take too long. Maybe we can also put together a short guide and see if an Agent can automate it :)

Regarding the second GH workflow (just in case it’s helpful) I was thinking of something similar to the format check we used to have. That action used reviewdog to perform the review and leave GH comments directly on the PR.

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.

4 participants