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

[Tracking] Code Spell-Checking Sprint #11984

Open
4 tasks
KlausLoeffelmann opened this issue Aug 25, 2024 · 15 comments · Fixed by #12154
Open
4 tasks

[Tracking] Code Spell-Checking Sprint #11984

KlausLoeffelmann opened this issue Aug 25, 2024 · 15 comments · Fixed by #12154
Labels
🚧 work in progress Work that is current in progress good first issue Issue should be easy to implement, good for first-time contributors help wanted Good issue for external contributors
Milestone

Comments

@KlausLoeffelmann
Copy link
Member

KlausLoeffelmann commented Aug 25, 2024

This is an approach suggestion for now, please feel free to comment.

Comments and variables in the source code still contain a LOT of typos or weird "spelling variations".
I would suggest, we create a list of areas (doing it all at the same time is WAY too invasive of course) for tracking, and then put the sub issues derived from that out there up-for-grabs, after we've defined, how we want to tackle this. Here is a rule list as a first suggestion:

  • Spellcheck/Formatting - don't do anything else: Sometimes you see a "potential quick change", like lambda-ing a property getter. Resist! 😸 Let's concentrate on spellchecking and reformatting, but not refactoring, no matter how tempting!
  • (Obvious) Typos inside of comments: Correct typos indicated by VS, but do not correct overzealously, meaning: If how an indicated typo to solve is not immediate clear to you, just leave it. If it's an impactful change (like a typo which you're not sure to correct appears 20 times in a source file), rather create a new issue.
  • Double-Space-Rule: Eliminate double-spaces behind a terminal punctuation, since this is a) a phased-out rule, which is no longer practiced, b) was US/English only and doesn't compute for an international audience and c) gets in the way, if it is applied accidentally inside of XML comments.
  • Typos inside of local variables: Correct obvious typos in local variables, if the names are NOT abbreviated. Also correct accidentally used camelcAsenAming (camelCaseNaming), but do not introduce camel case naming, unless you can answer "What would @JeremyKuhne or @lonitra do" confidently: The reason is, that we have a lot of Win32 API calls in the repo, and those variables have a naming standard which consist of a lot weird abbreviations and in addition is often just not camelCase, and not even camelCaseIsh.
  • Scope of other member items: NEVER change member names, which are protected, internal (Friend), virtual (Overridable) let alone public. If there is an obvious typo in such a member, please create a new issue so we can discuss, if we want to touch it (sometimes even changing an Internal member can be a breaking change, since ... you know ... Reflection...).
  • Control.cs (Sub-issue/PR)
  • Application.cs (Sub-issue/PR)
  • DataGridView (Sub-issue/PR)
  • [...to extend...] (Sub-issue/PR)

@Olina-Zhang, once we finalize this item and know what we want to do, we should take it over to the Designer repo, and - as suggestion - do the same?

@KlausLoeffelmann KlausLoeffelmann added this to the .NET 10.0 milestone Aug 25, 2024
@KlausLoeffelmann KlausLoeffelmann changed the title [Tracking] Code Spelling Sprint [Tracking] Code Spell-Checking Sprint Aug 25, 2024
@paul1956
Copy link
Contributor

paul1956 commented Aug 25, 2024

I have done this to all the VB code except possibly the 2 spaces after periods and 100% sentences with period in XML comments. The biggest offender is the resource and language files which I am not comfortable changing.

Also missing "see" attribute in XML Comments (how many and when).

What about changing attr to attribute in local names and other misspellings in local variables? VS considers anything 4 characters or longer misspelled if it's not in the dictionary. For words with special meaning/casing/spelling is there a way in a repo to add to spell checker?

I will take on finishing the VB code if no one else wants to when exact scope is clear.

@KlausLoeffelmann KlausLoeffelmann added the untriaged The team needs to look at this issue in the next triage label Aug 27, 2024
@merriemcgaw merriemcgaw added help wanted Good issue for external contributors and removed untriaged The team needs to look at this issue in the next triage labels Aug 27, 2024
@merriemcgaw merriemcgaw added the good first issue Issue should be easy to implement, good for first-time contributors label Aug 27, 2024
@paul1956
Copy link
Contributor

paul1956 commented Aug 29, 2024

@merriemcgaw how do I volunteer? I would like to try 1, There are two things that are easy to do, change local variable like attr to attribute (154 occurrences) it would include typeattr to typeAttribute and changing behaviour in behavior (29 occurrences) in comments. Also changing period with 2 spaces to period and 1 space.

Another issue causing a lot suggestions is below, I have no idea how to fix, should I just open a new issue?

Severity	Code	Description	Project	File	Line	Suppression State	Details
Message (active)	IDE0130	Namespace "System.Windows.Forms" does not match folder structure, expected "System.Windows.Forms.System.Windows.Forms.Accessibility"	System.Windows.Forms	C:\Repos\Winforms\src\System.Windows.Forms\src\System\Windows\Forms\Accessibility\AccessibleRoleControlTypeMap.cs	6		

@merriemcgaw
Copy link
Member

@paul1956 please go ahead and open a separate issue for the message above.

And I think you just volunteered 😉

@paul1956
Copy link
Contributor

paul1956 commented Aug 31, 2024

@merriemcgaw i will open the separate issue a namespace warning but not volunteering as its a C# issue and solving it is way beyond my C# skills.

I will open 2 separate issues (if necessary) on attrib and 2 spaces after period and volunteer for those. Also the biggest use of 2 periods after space is resource strings, I assume you don’t want me to touch those.

Where does one open issues about the Repo, none of the public issues seem to fit?

@paul1956
Copy link
Contributor

paul1956 commented Aug 31, 2024

@merriemcgaw C# has quoted strings with 2 periods after space. I think that is a separate issue that need to be opened. Also, XML Summy comments that are extremely long and don't wrap at column 120.

@merriemcgaw
Copy link
Member

@paul1956 I think you're right. Let's open a tracking issue on the content of quoted strings because I think that will take a bit more to do just right.

@paul1956
Copy link
Contributor

paul1956 commented Sep 2, 2024

@merriemcgaw

#12033 for period 2 spaced

The other tracking issues is extremely long XML comments that need to be wrapped by hand. #12034

The quoted strings/Resource file issue prevents an easy automated Regex fix, so I did it manually 370+ files changed.

@paul1956
Copy link
Contributor

paul1956 commented Sep 3, 2024

@merriemcgaw What do you want done about period space end-of-line or before XML comment close tag, some but not all, of these might be an artifact of period 2 spaces XML Close tag being corrected. Need to be careful because these also exist in Code/Tests so its a manual process.

lonitra pushed a commit that referenced this issue Sep 4, 2024
…ellings part of #11984 (#12028)

* Replaved period 2 spaces with period 1 space
Fixed a few obvious spelling errors
These shanges should only should ONLY be in comments, there are still 34 places in code and many in Resource files

* Undo accidential code change and wrong spelling fix.

* Correct selling of winform and  re-entrancy

* Fix 8 files that were incorrectly changed from period space to just period

* Update src/System.Windows.Forms/src/System/Windows/Forms/Layout/TableLayout.cs

Co-authored-by: Tanya Solyanik <[email protected]>

* PR Feedbak

---------

Co-authored-by: Tanya Solyanik <[email protected]>
@paul1956
Copy link
Contributor

paul1956 commented Sep 7, 2024

@merriemcgaw "Microsoft" is spelled as "microsoft" throughout repo (~663 URLS) as are some other words as part of a URL, URLs are (mostly) lower case and then in the display string where word is detected as misspelled, is also lower case. As part of a URL, it is not spell checked but as the display string it is. I see 3 options.

  1. Leave them as is and have ~600+ misspelled words
  2. Correct just the Display string (my recommendation), not what is being recommended in PR review.
  3. Change both, for consistency. URLs are not case sensitive so it's a easy solution.

This was referenced Sep 9, 2024
@merriemcgaw
Copy link
Member

I think your recommendation makes the most sense here. Let's do it.

@paul1956
Copy link
Contributor

I think your recommendation makes the most sense here. Let's do it.
@merriemcgaw

2 reviewers (@Epica3055 and @lonitra ) suggested
image

I am fine with any decision; I will go back at the end and do 1 PR to fix them all to whatever you want.

@Tanya-Solyanik
Copy link
Member

@paul1956 - would you be interested to do a pass over the comments to ensure XML tags are used where needed https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/recommended-tags

@paul1956
Copy link
Contributor

@Tanya-Solyanik yes but I need more information, on what you are looking for. Lists for example might be supported but they are really hard for a human to read. Adding "See" would be interesting but there is little guidance for how often it should be used (first occurrence only, every occurrence).

@paul1956
Copy link
Contributor

@Tanya-Solyanik I realized I spent so much time on shorting C# lines, my Regex missed VB. I will do that later to avoid merge issues.

@paul1956
Copy link
Contributor

@Tanya-Solyanik there are still many spelling issues. I got many of the ones in comments but not the ones involving private C# variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 work in progress Work that is current in progress good first issue Issue should be easy to implement, good for first-time contributors help wanted Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants