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

Check spelling 0.0.22 #16127

Merged
merged 11 commits into from
Dec 5, 2023
Merged

Check spelling 0.0.22 #16127

merged 11 commits into from
Dec 5, 2023

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Oct 11, 2023

Summary of the Pull Request

Upgrades check-spelling to v0.0.22

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

  • refreshes workflow
    • enables dependabot PRs to trigger CI (so that in the future you'll be able to see breaking changes to the dictionary paths)
  • refreshes metadata
  • built-in handling of \n/\r/\t is removed -- This means that the patterns/0_*.txt files can be removed.
  • this specific PR includes some shim content, in allow/check-spelling-0.0.21.txt -- once it this PR merges, it can be removed on a branch and the next CI will clean out items from expect.txt relating to the \r stuff and suggest replacement content.
  • talking to the bot is enabled for forks (but not the master repository)
  • SARIF reporting is enabled for PRs w/in a single repository (not across forks)
  • In job reports, there's a summary table (space permitting) linking to instances (this is a poor man's SARIF report)
  • When a pattern splits a thing that results in check-spelling finding an unrecognized token, that's reported with a distinct category
  • When there are items in expect that not longer match anything but more specific items do (e.g. microsoft vs. Microsoft), there's now a specific category with help/advice
  • Fancier excludes suggestions (excluding directories, file types, ...)
  • Refreshed dictionaries
  • The comment now links to the job summary (which includes SARIF link if available, the details view, and a generated commit that people can use if they're ok w/ the expect changes and don't want to run perl)

Validation Steps Performed

  1. the branch was developed in https://github.com/check-spelling-sandbox/terminal/actions?query=branch%3Acheck-spelling-0.0.22
  2. ensuring compatibility with 0.0.21 was done in Check spelling 0.0.22 check-spelling-sandbox/terminal#3
  3. this version has been in development for a year and has quite a few improvements, we've been actively dogfooding it throughout this period 😄

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@github-actions

This comment has been minimized.

@@ -6,8 +6,6 @@

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

:warning: The command is written for posix shells. If it doesn't work for you, you can manually _add_ (one word per line) / _remove_ items to `expect.txt` and the `excludes.txt` files.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the command should now work on Windows shells

Comment on lines 612 to +614
# the negative lookahead here is to allow catching 'templatesz' as a misspelling
# but to otherwise recognize a Windows path with \templates\foo.template or similar:
\\(?:necessary|r(?:eport|esolve[dr]?|esult)|t(?:arget|emplates?))(?![a-z])
#\\(?:necessary|r(?:eport|esolve[dr]?|esult)|t(?:arget|emplates?))(?![a-z])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is no longer necessary in 0.0.22 as I removed the built-in \n/\r/\t handling

\.zip$
^\.github/actions/spelling/
^\.github/fabricbot.json$
Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently fabricbot is gone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file can be removed once this PR merges, it's here to make the v0.0.21 engine happy. (I meant to include it in the initial PR, but missed a push...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(GitHub's definition of outdated is buggy, there's a github discussion where people complain about this poor behavior -- this comment is not outdated)

Comment on lines -46 to -47
tags-ignore:
- "**"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was never supported by pull_request_target as it / pull_request never applied to tags.

@@ -1,6 +1,7 @@
^attache$
^attacher$
^attachers$
^bellow$
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another archaic word I ran across... -- it's unlikely you'll want to reference stoking fires quite like this.

Copy link
Member

Choose a reason for hiding this comment

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

UGH THANK YOU

Comment on lines -67 to -68
# latex
\\(?:n(?:ew|ormal|osub)|r(?:enew)|t(?:able(?:of|)|he|itle))(?=[a-z]+)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as check-spelling no longer special cases \n/\r/\t, the workaround can be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as check-spelling no longer special cases \n/\r/\t, the workaround can be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as check-spelling no longer special cases \n/\r/\t, the workaround can be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as check-spelling no longer special cases \n/\r/\t, the workaround can be removed :)

@DHowett
Copy link
Member

DHowett commented Oct 19, 2023

AHH, I'm so excited!

We might-could need to revert the changes under dep/ and oss/. I'll noodle on that.

Thank you for doing this!

oss/stb/stb_rect_pack.h Outdated Show resolved Hide resolved
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
This file can be deleted after this branch is merged.
@DHowett
Copy link
Member

DHowett commented Dec 5, 2023

Okay, wow, I 100% didn't notice that you had done the oss/ and dep/ things. Dang it github! Force pushes are pushes too!

I'll review and summarily merge

This comment has been minimized.

not a XAML resource that controls this, nor is this something that's exposed by
no XAML resource that controls this, nor is this something that's exposed by
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I like that pattern!

@DHowett
Copy link
Member

DHowett commented Dec 5, 2023

Ugh, I shouldnae've merged main.

@DHowett DHowett merged commit dc986e4 into microsoft:main Dec 5, 2023
6 of 11 checks passed
zadjii-msft pushed a commit that referenced this pull request Dec 6, 2023
As noted by @jsoref in #16127, we could eventually remove this and also
check-spelling would make suggestions on what patterns to use.
@jsoref jsoref deleted the check-spelling-0.0.22 branch December 6, 2023 12:37
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.

None yet

3 participants