-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Fix a bunch of spelling errors across the project #4295
Conversation
@jsoref, is there a way we can run this as a part of our CI and make it fail on a typo like the clang-tidy style rules do? |
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.
Only change requested specifically is that we undo the ones for the internal renderers (bgfx and wddmcon) and fork that into a new issue so I can fix it upstream in the driver header too.
Also, if you have an idea how we could make a script to enforce this in the CI on our Windows build agents, I'm all for it. I think it would be great to do this like our style checks.
* `/src/cascadia/TerminalApp` - This DLL represents the implementation of the Windows Terminal application. This includes parsing settings, hosting tabs & panes with Terminals in them, and displaying other UI elements. This DLL is almost entirely UWP-like code, and shouldn't be doing any Win32-like UI work. | ||
* `/src/cascadia/WindowsTerminal` - This EXE provides Win32 hosting for the TerminalApp. It will set up XAML islands, and is responsible for drawing the window, either as a standard window or with content in the titlebar (non-client area). | ||
* `/src/cascadia/CasadiaPackage` - This is a project for packaging the Windows Terminal and its dependencies into an .appx/.msix for deploying to the machine. | ||
* `/src/cascadia/CascadiaPackage` - This is a project for packaging the Windows Terminal and its dependencies into an .appx/.msix for deploying to the machine. |
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.
Whoops. Good catch.
@@ -53,7 +53,7 @@ This feature will not impact reliability of Windows Terminal. | |||
|
|||
### Compatibility | |||
|
|||
With the implementation being mostly decoupled from the Windows Terminal app itself, no existing code/behaviours should break due to this feature. | |||
With the implementation being mostly decoupled from the Windows Terminal app itself, no existing code/behaviors should break due to this feature. |
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.
We would prefer to do the en-US
just to be consistent, generally speaking.
We've had our share of en-UK
from @bitcrazed dropped into assorted things, though. He very insistently says that Americans were given a perfectly good language that we decided to not use.
I don't mind the mix of them. But if you want to enforce one, lean toward en-US
.
@@ -57,7 +57,7 @@ namespace winrt::TerminalApp::implementation | |||
// the ctor, you're going to have a bad time. It'll mysteriously fail to | |||
// activate the AppLogic. | |||
// ALSO: If you add any UIElements as roots here, make sure they're | |||
// updated in _AppLogiclyTheme. The root currently is _root. | |||
// updated in _ApplyTheme. The root currently is _root. |
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.
Ugh. This was my fault. It was App --> AppLogic in an overzealous find/replace. I thought I fixed them all. So yeah. Apply
is the correct revert.
@@ -8,7 +8,7 @@ | |||
not try to read it. | |||
|
|||
Upstream problem report: | |||
https://developercommunity.visualstudio.com/content/problem/629524/static-library-reference-causes-there-was-a-proble.html | |||
https://developercommunity.visualstudio.com/content/problem/629524/static-library-reference-causes-there-was-a-problem.html |
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.
OK, good with me then.
{ | ||
const auto now = std::chrono::high_resolution_clock::now(); | ||
const std::chrono::duration<double> delta = now - _startTime; | ||
|
||
#pragma warning(suppress : 26477 26485 26494 26482 26446) // We don't control TraceLoggingWrite | ||
TraceLoggingWrite(g_hTerminalConnectionProvider, | ||
"RecievedFirstByte", | ||
TraceLoggingDescription("An event emitted when the connection recieves the first byte"), | ||
"ReceivedFirstByte", |
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.
Nnnnn. @amit-msft, does this mess up reporting if we correct the typo? Or can you glue the two together on the back-end?
const auto bbutton = L"\xD83C\xDD71"; | ||
position = bbutton; | ||
const auto bButton = L"\xD83C\xDD71"; | ||
position = bButton; |
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 think the author was trying to avoid it looking like the hungarian notation for "boolean Button" as bButton
. But it's a test, so I don't mind either way.
@@ -57,7 +57,7 @@ enum ECppCoreCheckWarningCodes | |||
WARNING_NO_CONST_CAST_UNNECESSARY = 26465, // Don't use const_cast to cast away const. const_cast is not required; constness or volatility is not being removed by this conversion (type.3: http://go.microsoft.com/fwlink/p/?LinkID=620419). | |||
WARNING_NO_STATIC_DOWNCAST_POLYMORPHIC = 26466, // Don't use static_cast downcasts. A cast from a polymorphic type should use dynamic_cast (type.2: http://go.microsoft.com/fwlink/p/?LinkID=620418). | |||
WARNING_NO_REINTERPRET_CAST_FROM_VOID_PTR = 26471, // Don't use reinterpret_cast. A cast from void* can use static_cast (type.1: http://go.microsoft.com/fwlink/p/?LinkID=620417). | |||
WARNING_NO_CASTS_FOR_ARITHMETIC_CONVERSION = 26472, // Don't use a static_cast for arithmetic conversions. Use brace initialization, gsl::narrow_cast or gsl::narow (type.1: http://go.microsoft.com/fwlink/p/?LinkID=620417). | |||
WARNING_NO_CASTS_FOR_ARITHMETIC_CONVERSION = 26472, // Don't use a static_cast for arithmetic conversions. Use brace initialization, gsl::narrow_cast or gsl::narrow (type.1: http://go.microsoft.com/fwlink/p/?LinkID=620417). |
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 was imported from somewhere, but it's fine to fix the typo.
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.
Looks like it came out of C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\VS\include\CppCoreCheck\warnings.h
but the typo looks fixed at least of VS 16.4.4 on my system.
@@ -248,10 +248,10 @@ bool WddmConEngine::IsInitialized() | |||
NewChar = &_displayState[rowIndex]->New[colIndex]; | |||
|
|||
OldChar->Character = NewChar->Character; | |||
OldChar->Atribute = NewChar->Atribute; | |||
OldChar->Attribute = NewChar->Attribute; |
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.
Nnnn. This one goes upstream. Can you revert and file a separate issue for any Atribute
to Attribute
that is on WddmCon*
or Bgfx*
related files (that is, any of them that are off of a CD_IO_CHARACTER
struct as I have to go fix that in the driver declaration inside Windows too.)
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 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, just those two (WddmConEngine
and BgfxEngine
) because they're special to bring-up editions of Windows that we don't ship. The other renderers don't run data through the driver in the CD_IO_CHARACTER
interface.
@miniksa : yes, for an example of this integration, see: https://github.com/checkstyle/checkstyle/blob/master/.ci/test-spelling-unknown-words.sh Note: You also need to pick a dictionary. (For licensing reasons, I don't host my own.) Ideally you want some sort of bucket which stores your previous set of whitelisted words. For checkstyle, that's: https://github.com/checkstyle/checkstyle/blob/master/.ci/jsoref-spellchecker/whitelist.words The right place to store the whitelist is probably a bucket (AWS S3/Azure Blob Storage/GCloud Bucket) -- You don't really want to version the whitelist in the repo as it's a large ever changing file that your users don't actually care about. I haven't written that code yet (I need to do it for my employer's project). |
files Heheheh... Also
Be sure you leave those out, for the most part. They come from somewhere else. If you want to make a list of the dependencies that have spelling errors, we can file issues to get them fixed upstream and in here at the same time. |
Hm, OK. We have a bucket in the form of the bucket we use to store our NuGet packages. We could keep a list there. We would also need to adapt your BASH script to likely a PowerShell one as that's what we'd have to run in the Windows Server CI. As for a dictionary, maybe we can use one that's installed with Visual Studio? I'd bet there's one in there somewhere and it'll already be on the build agents. Let's capture this stuff into another work item. |
I think at this point I've successfully dropped I can resurrect older versions of my changes to identify the things that could use fixing later once this is merged. I won't have any time to work on this until tonight. (It looks like it's dropping |
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.
The main thing I was worried about was TraceLoggingWrite()
and that's getting tracked by @miniksa's block. I'll approve so that once his comments get resolved, this can just go in. Thanks!
No problem. No rush. We're slow about PRing when we get busy; take your time fixing it up. Thanks for your effort. |
Well, I rebased, and I still haven't had a single successful run. It looks like they're still timing out :-( -- on the bright side, that's already covered by #4295 (reference) |
I'm working on fixing the test runs. I'll go look at the one you split out next. Thanks. Just the one comment left that CLI11 is an upstream dependency. So we should fix the typo there and consume (not here). |
Presuming the CI is happy now (tests should be better in master), I think we're good to go. Thank you. |
Hello @DHowett-MSFT! Because this pull request has the 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 (
|
🎉 Once again, thanks for the contribution! This pull request was included in a set of conhost changes that was just |
Generated by https://github.com/jsoref/spelling
f
; to maintain your repo, please considerfchurn
I generally try to ignore upstream bits. I've accidentally included some items from the
deps/
directory. I expect someone will give me a list of items to drop, I'm happy to drop whole files/directories, or to split the PR into multiple items (E.g. comments/locals/public).Closes #4294