Skip to content

Spellchecking - #5129

Closed
Deleted user (ghost) wants to merge 6 commits into
masterfrom
unknown repository
Closed

Deleted user (ghost) wants to merge 6 commits into
masterfrom
unknown repository

Conversation

@ghost

Copy link
Copy Markdown

Summary of the Pull Request

Since we are now doing spellchecking, I thought I'd help out.

References

#4799

PR Checklist

  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed

Detailed Description of the Pull Request / Additional comments

Spell Checks

Validation Steps Performed

@zadjii-msft Mike Griese (zadjii-msft) left a comment

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.

Should we remove "colortable", "PowerShell" from the whitelist/dictionary to make sure that we get all these cases?

@ghost

Copy link
Copy Markdown
Author

Yes let us remove those

@ghost

Copy link
Copy Markdown
Author

except some commands use the lowercase variant, and those commands are documented here, so... that is where things get tricky

@jsoref

Josh Soref (jsoref) commented Mar 26, 2020 •

Copy link
Copy Markdown
Contributor

pi1024e: You can add entries into patterns to specifically accept a phrase (technically text matching patterns you add are removed before it parses things looking for unrecognized items).

An example pattern:

\b(?:)powershell -noprofile\b

I'm going to have to change how the output works, in theory you'd want to be told to remove entries from the whitelist, but right now you'll only see that if you add one misspelling or if you run w/ debug... For the time being, I'd suggest making an extra local branch in your repository on top of this branch and having it have a single misspelling -- you'd never create a PR from this branch nor merge it. You can then use the output to remove whitelist entries from this branch.

I was trying to avoid having the bot be too chatty, but I think people will probably be ok (or at least you would) w/ being told how to remove things from the whitelist.

@jsoref

Copy link
Copy Markdown
Contributor

Right now the spell checker is actively ignoring .yml files. You could work on removing that exclusion:

I think the reason I excluded .yml files is that they often have shas in them, but a glance at this repository indicates that they don't, so one could (should?) remove that exclusion.

@zadjii-msft Mike Griese (zadjii-msft) added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Mar 29, 2020

For example, the WSL generator would return `Microsoft.Terminal.WSL`. The
Powershell Core generator would return `Microsoft.Terminal.PowershellCore`.
PowerShell Core generator would return `Microsoft.Terminal.PowershellCore`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
PowerShell Core generator would return `Microsoft.Terminal.PowershellCore`.
PowerShell Core generator would return `Microsoft.Terminal.PowerShellCore`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You probably don't want that extra blank line. GitHub changed the default behavior of suggestion such that on average suggestions include one -- fairly annoying.


// Method Description:
// - Adds the given colorscheme to our map of schemes, using its name as the key.
// - Adds the given ColorScheme to our map of schemes, using its name as the key.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't believe this needs to be capitalized as the object name. I think the generic "color scheme" terminology was sufficient.

@zadjii-msft Mike Griese (zadjii-msft) added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 31, 2020
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Apr 7, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@ghost ghost closed this Apr 14, 2020
@ghost
ghost deleted the Spelling branch April 29, 2020 16:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants