-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Upgrade to check-spelling v0.0.22 #3896
Conversation
# Because it doesn't handle argument -Words well | ||
^tools/CorrelationTestbed/.*\.ps1$ | ||
^tools/COMTrace/ComTrace.wprp$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaict this is no longer a problem, so I'm dropping it
.github/actions/spelling/expect.txt
Outdated
TOperation | ||
TOptions | ||
TProgress | ||
TResult | ||
TReturn | ||
trimstart | ||
TState | ||
TStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These T[A-Z]
items are handled by a pattern below.
if: "contains(github.event_name, 'pull_request') || github.event_name == 'push'" | ||
if: ${{ contains(github.event_name, 'pull_request') || github.event_name == 'push' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is purely stylistic -- the vscode github integration doesn't like "..."
whereas GitHub Actions do not mind.
.github/workflows/spelling3.yml
Outdated
extra_dictionaries: | ||
cspell:cpp/src/compiler-msvc.txt | ||
cspell:cpp/src/cpp.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this dictionary is lousy (way too many typos included, similar to the win32 dictionary) -- as of v0.0.22, I 🗑️ Removed (both) Dictionaries from the suggestion list due to their poor quality.
I did look into trying to fix them, but they had so many typos that I ran out of energy trying to go over them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why you wouldn't want to suggest the lousy dictionaries, but I'm not totally convinced it is better for us to not use them. I'm not too invested in that position, though.
The downside I see to using them is that if we happen to make the same typo, we wouldn't notice. To me that seems somewhat unlikely, and if it does happen doesn't seem like that big of a deal. I assume the dictionary would get better over time, so those typos would eventually come up.
As for upsides, it will be easier for us to manage the lists of allow/expect if we have to add less words. If we have to add lots of words, I think it's more likely we're going to make another mistake like with "uknown"
.github/workflows/spelling3.yml
Outdated
experimental_apply_changes_via_bot: ${{ github.repository_owner != 'microsoft' && 1 }} | ||
|
||
update: | ||
name: Update PR | ||
permissions: | ||
contents: write | ||
pull-requests: write | ||
actions: read | ||
runs-on: ubuntu-latest | ||
if: ${{ | ||
github.repository_owner != 'microsoft' && | ||
github.event_name == 'issue_comment' && | ||
github.event.issue.pull_request && | ||
contains(github.event.comment.body, '@check-spelling-bot apply') | ||
}} | ||
concurrency: | ||
group: spelling-update-${{ github.event.issue.number }} | ||
cancel-in-progress: false | ||
steps: | ||
- name: apply spelling updates | ||
uses: check-spelling/check-spelling@v0.0.22 | ||
with: | ||
experimental_apply_changes_via_bot: 1 | ||
checkout: true | ||
ssh_key: "${{ secrets.CHECK_SPELLING }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is, as noted, very optional, but it should make it easier for others to interact with the workflow instead of having to run programs on their own computer.
0a4ca44
to
64d4149
Compare
.github/workflows/spelling3.yml
Outdated
extra_dictionaries: | ||
cspell:cpp/src/compiler-msvc.txt | ||
cspell:cpp/src/cpp.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why you wouldn't want to suggest the lousy dictionaries, but I'm not totally convinced it is better for us to not use them. I'm not too invested in that position, though.
The downside I see to using them is that if we happen to make the same typo, we wouldn't notice. To me that seems somewhat unlikely, and if it does happen doesn't seem like that big of a deal. I assume the dictionary would get better over time, so those typos would eventually come up.
As for upsides, it will be easier for us to manage the lists of allow/expect if we have to add less words. If we have to add lots of words, I think it's more likely we're going to make another mistake like with "uknown"
64d4149
to
c1fe3a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot allow write permissions.
c1fe3a1
to
d776ec2
Compare
We use a "template: foo/bar/baz" type of convention for messages from bots so it's easy to build e-mail rules to filter those out. How hard would it be to add something like that to the spellcheck messages in PRs? |
@denelon, add a line to https://github.com/microsoft/winget-cli/blob/master/.github/actions/spelling/advice.md You could even add it of the form:
So that it's only seen by mail filters and not by humans. |
what's the latest update on this? |
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
6236f70
to
051f9ba
Compare
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> spelling: contoso 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>
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>
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>
051f9ba
to
7a6055b
Compare
@@ -23,3 +25,6 @@ https://www.regexplanet.com/advanced/perl/) yours before committing to verify it | |||
Note that patterns can't match multiline strings. | |||
|
|||
</details> | |||
<!-- | |||
template: check-spelling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per #3896 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denelon I hope this meets your needs, but you're welcome to change it to some other pattern.
@@ -21,7 +21,7 @@ Installers: | |||
Language: /en-US | |||
Custom: /s | |||
- Arch: x64 | |||
Url: https://contosa.net/publiccontainer/contosainstaller64.exe | |||
Url: https://contoso.com/publiccontainer/contosoinstaller64.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd been ignoring this for a while... contoso.com is microsoft's domain. contosa is a typo of contoso, and contosa.net isn't registered.
If this needs to not be contoso.com, it should be an example.com or some other microsoft controlled domain...
Hmm.. I guess I need to rebase -- I was hoping @denelon or @JohnMcPMS would have reviewed it since... At this point, I'm close to releasing the next version... |
Sorry about that! We've been super busy with some internal work and haven't been as good as we should have been about reviewing external PRs. Do you want me to get someone to review this or hold off until the next release? |
7a6055b
to
8a51af7
Compare
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
8a51af7
to
d92289f
Compare
comment-push: | ||
name: Report (Push) | ||
# If your workflow isn't running on push, you can remove this job | ||
runs-on: ubuntu-latest | ||
needs: spelling | ||
permissions: | ||
contents: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing this job from the flow.
In the long term, I'm nudging people to rely on the step summary (which happens automatically) over adding comments, but in the near term, people can rely on comments when they make a pull request.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@JohnMcPMS could you add
|
@jsoref, I just added it. |
Changes:
cspell:cpp/src/cpp.txt
, but you could use it orcspell:cpp/dict/cpp.txt
-- I think I missed that Add suggested dictionaries for spellchecking and remove unneeded words #3885 was adding it -- cpp suffers from the same problem as win32 -- too many misspelled words)Microsoft Reviewers: Open in CodeFlow