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

Remove "wether" from dictionary, add "VTE" #5207

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

zadjii-msft
Copy link
Member

Technically, "wether" is a word but I'd be shocked if there's a scenario for us to use it properly in this repo, so I'm pulling it from the dictionary.

Also, in #5200, we added "VTE", which is totally a valid acronym, to the codebase, but not the whitelist. I'm not sure why the bot let me merge it anyways, but I'm fixing it now.

@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Apr 1, 2020
@DHowett-MSFT
Copy link
Contributor

Hold up, I’m not sure we should be changing the dictionary itself. I think it’s supposed to be representative of English. /cc @jsoref to keep me honest

@jsoref
Copy link
Contributor

jsoref commented Apr 1, 2020

In general, you shouldn't be adding to dictionary.txt, you can add custom words to any .txt file in https://github.com/microsoft/terminal/tree/master/.github/actions/spell-check/dictionary

As for removing items, if you want to use a subset of the dictionary because it's too likely that people will use a word that, while technically an English word, is not actually the one people should be using, then, yes, removing it from dictionary.txt would be reasonable.

@jsoref
Copy link
Contributor

jsoref commented Apr 1, 2020

As for why the check didn't block things, that's a bit of a mess:

j4james/terminal#clamp-vt-parameters got a fail from the bot when j4james@d2690a0 was pushed to @j4james 's repository which triggered d2690a0/comment which is j4james@d2690a0#commitcomment-38185159 which GitHub also renders as #5200 (comment) .

I've fixed the schedule code so that there's a way to ensure comments exist (unfortunately, that check wouldn't appear as an ❌ against the PR, because it runs in response to a different event).

I'm planning to write code so that the bot will "resolve" older comments when it runs again. -- I've started thinking about this code.

I could probably have it add/remove a "review" thing for "changes requested". -- I haven't looked into this yet.

I don't think I'll be able to implement the unwritten bits before mid-April, but if you update to the current release (0.0.13-alpha), you can at least use advice to remind reviewers to make sure that the comments are addressed. I could make a PR to update this repo today. advice is now merged which will hopefully help a little.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 1, 2020
@ghost
Copy link

ghost commented Apr 1, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours 59 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett-MSFT DHowett-MSFT merged commit 9409e85 into master Apr 1, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/migrie/b/fix-spellcheck-bot branch April 1, 2020 19:15
@j4james
Copy link
Collaborator

j4james commented Apr 1, 2020

FYI, I definitely got a notification of the VTE spelling issue. I just didn't know how to go about whitelisting it, and I had intended to follow up on that, but the PR got merged a bit faster than I had expected.

@DHowett-MSFT
Copy link
Contributor

Sorry about that 😄

@zadjii-msft
Copy link
Member Author

@j4james no worries, I thought it had been resolved since the PR wasn't blocked, but it's nbd. We've merged much worse problems into master before 😅

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. AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants