- Notifications
You must be signed in to change notification settings - Fork 344
Refresh check-spelling#9632
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
Refresh check-spelling #9632
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| ^pkg/rancher-desktop/assets/scripts/logrotate-k3s$ | ||
| ^pkg/rancher-desktop/assets/scripts/logrotate-lima-guestagent$ | ||
| ^pkg/rancher-desktop/sudo-prompt/ | ||
| ^pkg/rancher-desktop/utils/processOutputInterpreters/__tests__/assets/trivy-image-postgres |
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.
There are a pair of files that had a bunch of things that check-spelling doesn't like, but they're clearly designed as inputs to a test based on real world values, so fixing them doesn't make sense.
| ipaddr | ||
| IPAM | ||
| IPlugin | ||
| iptable |
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 removed a pattern for this because it was the only case covered and it didn't seem worth it for me to carry along the cost while checking all lines in all files.
| XVar | ||
| xvfb | ||
| xwininfo | ||
| xyzzy |
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 not quite sure what changed that resulted in this being seen.
| # hit-count: 2 file-count: 1 | ||
| # iSCSI iqn (approximate regex) | ||
| \biqn\.[0-9]{4}-[0-9]{2}(?:[\.-][a-z][a-z0-9]*)*\b:[\w.]+ |
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 slightly extended from the base pattern
| inputs: | ||
| skip-electron: | ||
| description: Skip Electron | ||
| default: '' | ||
| required: false | ||
| skip-go: | ||
| description: Skip Go | ||
| default: '' | ||
| required: false | ||
| skip-python: | ||
| description: Skip Python | ||
| default: '' | ||
| required: false |
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.
As noted, these changes allow check-spelling to bootstrap 30s faster
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.
And we can probably use them in other workflows too. 👍
(No need to make that change in this PR, of course.)
| FEATURE: 'management.cattle.io.feature', | ||
| GROUP: 'management.cattle.io.group', | ||
| KONTANIER_DRIVER: 'management.cattle.io.kontainerdriver', | ||
| KONTAINER_DRIVER: 'management.cattle.io.kontainerdriver', |
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.
notable
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.
And this will probably get clobbered next time we re-import the file from dashboard (we never use this).
| check_file_names: 1 | ||
| post_comment: 0 | ||
| use_magic_file: 1 | ||
| ignore-next-line: spell-checker:disable-next-line |
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 won't do anything yet. It should be supported in the next version
| // builderCacheProcessor implements the details for handling the argument for | ||
| // `nerdctl builder build --cache-from=...` and | ||
| // `nerdctl builder builder --cache-to=...` | ||
| // `nerdctl builder build --cache-to=...` |
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.
notable. check-spelling didn't like this, but I don't think nerdctl did either...
package-spelling.json Outdated
| @@ -0,0 +1,26 @@ | |||
| { | |||
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 file is used by the workflow (it has no dependencies).
I may end up providing a version of this in the check-spelling repository, as it seems reasonably close to something people might want.... I still need to think about how this should work.
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 basically there so yarn installdoesn't actually do much, right?
Is that needed, or can we get away with skipping .github/actions/yarn-install/action.yaml? (Just do setup-node without caching + corepack enable)
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 have no idea. I didn't write the workflow bits that use it. I figured I could safely skip the bits I found I could skip, but I wasn't really sure whether it was a policy that yarn-install be used everywhere or....
-- I probably should have asked.
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.
It was just there because we got tired of copying bits and pieces across different workflows (especially when we added things that were needed). In this case, I think it's fine to not use it.
yarn-spelling.lock Outdated
| @@ -0,0 +1,12 @@ | |||
| # This file is generated by running "yarn install" inside your project. | |||
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 corresponds to the package-spelling.json file.
9479f52 to 8836427Compare
mook-as left a 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.
I'll need to double check if we can skip the whole alternate package.json thing and just not run yarn install for check-spelling… otherwise it's mainly pretty trivial things.
You may want to just skip the files we import from dashboard though, or make PRs upstream.
I'll approve or request changes depending on how my experiments with package.json go.
| \bgitlab\.[^/\s"]*/(?:[^/\s"]+/){2}commits?/[0-9a-f]+\b | ||
| # #includes | ||
| ^\s*#include\s*(?:<.*?>|".*?") |
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.
Hmm, it would be nice if the patterns could be limited to specific file paths, so #includes only for *.h/*.c/*.hpp/*.cpp/*.cc. Same with uses: only in .github/**/*.yaml.
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.
Yeah... It's a thing that comes up occasionally, but then I'd have to make the file format much more complicated.
Uh oh!
There was an error while loading. Please reload this page.
| \b2006-02-01\b | ||
| # Should probably have a trailing `.` | ||
| \s([a-z]\.){2,}[a-z]\s |
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 don't see this being hit a lot (a.b.c instead of a.b.c., where all of those things are single-letters), but okay
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.
| # s.b. macOS or Mac OS X or ... | ||
| # Should be `Jira` | ||
| \bJIRA\b |
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.
Unless you're tracking things you are doing to manage Jira within itself. This should be \bJIRA\b(?!-\d+) or something along those lines, to avoid flagging JIRA-12345.
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.
Thanks. I've fixed it in check-spelling/spell-check-this@9d03041 for the baseline.
| # hit-count: 739 file-count: 6 | ||
| # base64 encoded content, possibly wrapped in mime | ||
| (?:^|[\s=;:?])[-a-zA-Z=;:/0-9+]{50,}(?:[\s=;:?]|$) | ||
| NO_VOWELS:\s+'.+' |
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.
| NO_VOWELS: 'bcdfghjklmnpqrstvwxz2456789', |
That's from dashboard and we don't really care.
Uh oh!
There was an error while loading. Please reload this page.
| FEATURE: 'management.cattle.io.feature', | ||
| GROUP: 'management.cattle.io.group', | ||
| KONTANIER_DRIVER: 'management.cattle.io.kontainerdriver', | ||
| KONTAINER_DRIVER: 'management.cattle.io.kontainerdriver', |
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.
And this will probably get clobbered next time we re-import the file from dashboard (we never use this).
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.
All the changes will probably be clobbered next time we re-import from dashboard.
You may be interested in pushing this to https://github.com/rancher/dashboard (specifically https://github.com/rancher/dashboard/blob/master/shell/components/SortableTable/index.vue ) but definitely extra; feel free to ignore.
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.
Hmm, I sent them PRs in 2022. I suppose I could again....
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.
Note that we (for the most part) don't actually use and translations; all of the changes here are for dashboard strings.
| // This removes it from the layout of ButtondropDown (so it doesn't render huge for SSR) but | ||
| // still allows it to maintain it's dimensions for v-tooltip to calculate the appropriate position. | ||
| // This removes it from the layout of ButtonDropDown (so it doesn't render huge for SSR) but | ||
| // still allows it to maintain its dimensions for v-tooltip to calculate the appropriate position. |
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 have no idea either, it's been here since the initial PoC stages. I suspect the rules aren't really doing anything useful.
mook-as left a 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.
Poked about it for a bit, and I think I prefer not having an extra package.json lying around.
My experiment was 6e5b6a7
The core is basically:
yq --inplace '.dependencies ={} | .devDependencies ={}' package.json rm -f yarn.lock corepack enable yarn yarn install --no-immutable --mode=skip-buildAll that together is <5s, so that should be fine? It just ends up installing nothing (other than yarn itself), and skips the postinstall script to avoid failures. Installing cpanminus takes about 20s, and the spell check itself is about a minute.
Co-authored-by: Mark Yen <[email protected]> Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Co-authored-by: Mark Yen <[email protected]> Signed-off-by: Josh Soref <[email protected]>
3a466cd to f509117Compare
mook-as left a 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.
Thanks for bearing with me on all the nit picks!
I'll note that this does not bump the version of check-spelling we use:
| check-spelling: 0.0.25 |
But that's probably a function of not having a release yet, and that's perfectly fine.
aff2928 into rancher-sandbox:mainUh oh!
There was an error while loading. Please reload this page.
This mostly updates the check-spelling metadata.
It also prepares for the next version (applying newer dictionaries that I hope to eventually switch to).
In general, I'd recommend reviewing starting with changes to expect.txt -- new entries should be considered, they probably indicate changes to patterns or dictionaries. I haven't thought too carefully about all of them, but I'll highlight some.
I'm also tossing in a change to the yarn-install behavior because the workflow in github currently burns >30s installing stuff that it won't use.