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 to check-spelling v0.0.22 #2316

Merged
merged 14 commits into from
Oct 26, 2023
Merged

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Oct 13, 2023

Originating Project/Creator
Affected Component CI
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Upgrades check-spelling to v0.0.22

Rationale

  • Adds sarif output (but not for PRs across forks)
  • Improves handling for patterns that split words
  • \n/\r/\t are no longer special cased
  • Fancier excludes suggestions (including directories, filenames, file extensions)
  • Future @dependabot PRs will trigger proper CI to catch when dictionaries are reshuffled
  • Updated dictionaries
  • In forks, users will be able to talk to the bot to update metadata
  • Candidate patterns that look for = won't trip on current patterns

https://github.com/check-spelling/check-spelling/releases/tag/v0.0.22

Testing/Review Recommendations

PRs can be made into https://github.com/check-spelling-sandbox/fprime/tree/check-spelling-0.0.22 to see how it behaves.

Future Work

.github/actions/spelling/allow.txt Outdated Show resolved Hide resolved
.github/actions/spelling/patterns.txt Show resolved Hide resolved
.github/actions/spelling/patterns.txt Show resolved Hide resolved
.github/actions/spelling/patterns.txt Outdated Show resolved Hide resolved
docs/Tutorials/CrossCompilationSetup/macOS.md Show resolved Hide resolved
docs/Tutorials/CrossCompilationSetup/macOS.md Show resolved Hide resolved
docs/UsersGuide/dev/baremetal-multicore.md Show resolved Hide resolved
docs/UsersGuide/user/fprime-util.md Show resolved Hide resolved
Autocoders/Python/schema/testSchemas.py Show resolved Hide resolved
@jsoref jsoref force-pushed the check-spelling-0.0.22 branch 2 times, most recently from 453ac20 to 3375800 Compare October 13, 2023 09:13
Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Couple of questions, mostly about the change in the workflow file

.github/actions/spelling/expect.txt Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/actions/spelling/expect.txt Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Hey @jsoref ! Thank you very much for this PR, really appreciate it. This is looking great.

One thing we'd like to change: there are security concerns that make me a little uneasy about allowing the permissions that are required for the new bot interaction feature that is introduced in this new release. I understand that it's only running in external repositories, but still I think we're going to want to disable it.
I have added comments where I think we need to make the changes - let me know if you have any comments/questions/concerns, or see anything else.

Also I'll gladly make those changes myself, I just thought maybe you'd want to author that yourself.
Thank you very much again!

.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Outdated Show resolved Hide resolved
.github/workflows/spelling.yml Show resolved Hide resolved
@jsoref
Copy link
Contributor Author

jsoref commented Oct 26, 2023

Oh, lemme rebase...

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>
- enables sarif reporting
- enables checking dependabot PRs to validate extra_dictionaries in the
    future
- refreshes dictionaries
- refreshes metadata
@@ -547,7 +547,8 @@ customObjectInstantitationMethod
# base64 encoded content
([`'"])[-a-zA-Z=;:/0-9+]+=\g{-1}
# base64 encoded content in xml/sgml
>[-a-zA-Z=;:/0-9+]+=</
# Commented out because of false positives
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this false positive is only present in v0.0.21 and will go away in v0.0.22 (one of the many bugs that are fixed). But it doesn't matter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah my bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have called it out in the PR description.

In the release notes, it's listed as:

🐜 Bug fixes
🩹 Pattern suggestions shouldn't trip over active patterns

@LeStarch LeStarch merged commit 9633049 into nasa:devel Oct 26, 2023
34 checks passed
@jsoref jsoref deleted the check-spelling-0.0.22 branch October 26, 2023 20:53
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