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

Upgrade check-spelling to v0.0.21 #2728

Merged
merged 10 commits into from
Dec 2, 2022

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Nov 29, 2022


Upgrades check-spelling to v0.0.21

The command to apply changes should now work on Windows (it requires Perl, but I believe that's more or less present most of the time, and it should walk you through the rest of the required tools).

There are a bunch of new features, the most important here are probably being able to update the metadata from Windows. (If it doesn't work, please @ me).

Also, candidate.patterns will automatically suggest patterns. You can see them in patterns.txt, e.g.:

# hit-count: 4 file-count: 4
# Non-English
[a-zA-Z]*[ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖØÙÚÛÜÝßàáâãäåæçèéêëìíîïðñòóôõöøùúûüýÿĀāŁłŃńŅņŒœŚśŠšŜŝŸŽžź][a-zA-Z]{3}[a-zA-ZÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖØÙÚÛÜÝßàáâãäåæçèéêëìíîïðñòóôõöøùúûüýÿĀāŁłŃńŅņŒœŚśŠšŜŝŸŽžź]*

The metadata bits (the hit count/file count) don't have to be retained (I hope they'll be useful in deciding whether/or not to add a pattern, i.e. "how applicable is it?"), the comment hinting at what the pattern does is probably worth retaining.

Extra cleanup

There are too many release branches pointing to the previous version, so I'm leaving a stub for it present as I did before.

Microsoft Reviewers: Open in CodeFlow

@jsoref jsoref requested a review from a team as a code owner November 29, 2022 12:23
@jsoref jsoref force-pushed the check-spelling-v0.0.21 branch from 9c765a2 to 3a86bef Compare November 29, 2022 12:31
NOTICE Outdated
@@ -23,7 +23,6 @@ Castle.Core 4.4.0 - Apache-2.0
(c) 2008 VeriSign, Inc.
Copyright 2004-2016 Castle Project
Copyright (c) 2004-2019 Castle Project
GCopyright (c) 2004-2019 Castle Project
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 line appears to be damage from an errant sed command.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to make the change in this pr at the moment, but this NOTICE is auto generated, it will be overridden whenever we have new components added. I think we should add this notice to exclude list if spell checking is not happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the file is generated from other things beyond your control, then excluding it is probably the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know where this item comes from? I'd like to fix it at the source...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is generated for each build in the build pipeline. For example, for this pr build, the notice is generated here https://dev.azure.com/ms/winget-cli/_componentGovernance/156969?_a=components&typeId=14063008&alerts-view-option=active
This is from internal Component Governance Devops feature owned by internal compliance team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you pass them a note suggesting that their data record for this item is probably buggy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll file a bug for the right team to take a look.

@@ -1157,7 +1156,7 @@ Copyright (c) 2006-2013 Alexander Chemeris
Copyright (c) The Internet Society (2005).
Copyright (c) The Internet Society (2006).
copyright 2009-2013 Christopher M. Kohlhoff
Copyright (c) 2016, Akamai Technolgies, Inc.
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 appears to be a typo (Google finds fewer than 500 hits for this spelling, although, admittedly at least one is in a proxy statement to the SEC, but even there the correct spelling is also present...).

@@ -19,6 +24,12 @@
# s.b. greater than
\bgreater then\b

# s.b. into
#\sin to\s
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 this is not always right, I'm not leaving it active.

[Rr]e[- ]entrant

# s.b. workaround(s)
#\bwork[- ]arounds?\b
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, I'm leaving this out. It's right some of the time (including once below).

azure-pipelines.loc.yml Show resolved Hide resolved
var testreuslt = TestCommon.RunAICLICommand("install", $"{packageId}");
var testResult = TestCommon.RunAICLICommand("install", $"{packageId}");
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 worrisome as it appears to be unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll let one of the engineers review this to see if it's used in some back-end tests.

@jsoref jsoref force-pushed the check-spelling-v0.0.21 branch from 3a86bef to 45099f0 Compare November 29, 2022 12:36
Comment on lines +30 to +31
# s.b. opt-in
#\sopt in\s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, this isn't always right, and in fact the one case I ran into here was a bad example... (Some heuristics are really not good enough to leave lying around, and instead handling the associated cases should be left to a grammar checker that can be truly context aware...)

@denelon
Copy link
Contributor

denelon commented Nov 29, 2022

@jsoref thanks for raising the PR. I'll have the engineering team review and merge.

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Trenly
Copy link
Contributor

Trenly commented Nov 30, 2022

@jsoref - Would you mind updating the action at winget-pkgs also?

@jsoref
Copy link
Contributor Author

jsoref commented Dec 2, 2022

(For the record: microsoft/winget-pkgs#90103 (review))

@yao-msft
Copy link
Contributor

yao-msft commented Dec 2, 2022

Files under src/purelib are subtree from https://github.com/ronomon/pure
Files under src/XLang are subtree from https://github.com/microsoft/xlang

Please revert the changes in these files and exclude the 2 projects from spell checking.

And thank you @jsoref for bringing the v0.0.21, we can finally reenable the spell check again.

@jsoref jsoref force-pushed the check-spelling-v0.0.21 branch from 45099f0 to 04585b9 Compare December 2, 2022 05:41
@yao-msft
Copy link
Contributor

yao-msft commented Dec 2, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft yao-msft merged commit 4b8e189 into microsoft:master Dec 2, 2022
@jsoref jsoref deleted the check-spelling-v0.0.21 branch December 3, 2022 22:41
@jsoref jsoref mentioned this pull request Dec 3, 2022
2 tasks
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