Skip to content

Spell check - #645

Merged
JohnMcPMS merged 34 commits into
microsoft:masterfrom
jsoref:spell-check
Nov 16, 2020
Merged

JohnMcPMS merged 34 commits into
microsoft:masterfrom
jsoref:spell-check

Conversation

@jsoref

@jsoref Josh Soref (jsoref) commented Nov 14, 2020 •

Copy link
Copy Markdown
Contributor

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
@jsoref
Josh Soref (jsoref) requested a review from a team as a code owner November 14, 2020 23:52
@ghost ghost added the Issue-Docs It's a documentation issue that really should be on MicrosoftDocs label Nov 14, 2020

@jsoref Josh Soref (jsoref) left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's possible to split the spelling fixes from the spell-check action, but it's important to land the spell-checker w/o letting there be too much drift between its PR base and master -- it's bad to land it if there have been typos introduced since its base (as all new PRs from its landing will have confusing failures).

For reference, this PR gets a ✅ : jsoref@4ad3543

<data name="SourceListField" xml:space="preserve">
<value>Veld</value>
<comment>The name of a peice of information about a source.</comment>
<comment>The name of a piece of information about a source.</comment>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's possible I should drop the localization comment changes (I never know what the policy is)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Certainly the localization comments can be updated; they will just be overwritten by future changes if needed.


CompletionTestCommand command;
command.SubCommandNames = { "car", "cart", "cartesion", "carpet" };
command.SubCommandNames = { "car", "cart", "cartesian", "carpet" };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change is probably the biggest

{
// If moved to C++20, this can be replaced with standard library implementations.
void OutputTimepoint(std::ostream& stream, const std::chrono::system_clock::time_point& time)
void OutputTimePoint(std::ostream& stream, const std::chrono::system_clock::time_point& time)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given that the underlying type is time_point, it made sense to me to use this capitalization

return "winget upgrade --all completed with failures";
default:
return "Uknown Error Code";
return "Unknown Error Code";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This could be a cli api change

if (!availablePackage)
{
AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined, setting availablility state to unknown");
AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined, setting availability state to unknown");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This may be a cli api change

// DisplayVersion
// Version
// MajorVerison, MinorVersion
// MajorVersion, MinorVersion

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Google Sheets suggested Verizon

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Major Verizon

A lot of these changes are my extremely common typos that I failed to correct. I'm sure that some sort of ML (or just analysis of qwerty) could probably figure out that one of my fingers jumps the gun on the others occasionally.

Comment thread .github/actions/spelling/excludes.txt Resolved
Add JsonCppLib
Revert spelling changes to JsonCppLib.
- uses: actions/checkout@v2.0.0
with:
fetch-depth: 5
- uses: check-spelling/check-spelling@0.0.16-alpha

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

check-spelling/check-spelling@0.0.16-alpha [](start = 12, length = 42)

I'm not familiar with GitHub Actions.

  1. Will we need to update this version manually to keep up with new releases?
  2. Do we need to do something to manage the secrets.GITHUB_TOKEN? Or is that automatically done by the Actions infrastructure?
  3. Generally, are there any other actions we need to take to manage this (beyond excludes, patterns, etc. for new changes)?

Thanks for your work here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. In theory Dependabot (@dependabot) can tell you about updates, but for the near term, I'll probably make the PR myself.
  2. The GITHUB_TOKEN is automatic https://docs.github.com/en/free-pro-team@latest/actions/reference/authentication-in-a-workflow#about-the-github_token-secret
  3. Beyond that and trying to avoid merging things with spelling errors (if the main branch is unhappy, anyone who branches from that point will be unhappy / confused), no.
    • There is the option to add an advice.txt file which is included in the message the bot leaves, but that's totally tunable and that's really something you'll work through as a project.
    • Technically GitHub has a budget for actions, it's theoretically possible for someone to run into this limit, I suspect that Microsoft has a reasonably large budget and you won't notice (certainly microsoft/terminal hasn't complained) -- if someone publicly runs into this, I'd love to hear about it. (Generally other actions are more expensive than this one -- which runs at about 1 minute and is thus billed as 1-2 units.) Most likely if a project like yours hits something like this, it means you have a bunch of files that should be ignored that you aren't ignoring (one project I'm looking at takes ~20 minutes to scan). -- If this becomes a problem for some other project in the future, I'll probably write some code so that it tries to scan only changed files instead of the whole repository.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the info, I agree that only checking changed files seems like a good optimization opportunity.

@JohnMcPMS

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
JohnMcPMS merged commit 0e1e0b4 into microsoft:master Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue-Docs It's a documentation issue that really should be on MicrosoftDocs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SpellChecker to GH Action

2 participants