Code cleanup only and Add VB Tests

[paul1956]

Fixes part of #10090 Standalone PR to do some cleanup of VB Code Removes VB Option from source files and moves to Project File Correct Spelling and Capitalization Errors Add a few tests Update editorConfig to support converting 3 NotInheritable classes to Modules. Add a few tests Change format of XLM Comment to indent text by 1 space Change a few Private functions to Friend for testing Not all issues are addressed in files I expect to replace. Not done is reorganizing code in files to be in standard order, this will be a follow-on PR

Proposed changes

Fix above with very minimal code changes

Customer Impact

• None only developers will notice

Regression?

No

Risk

• Minimal due to almost no code changes

Test methodology

Added tests where required otherwise covered by existing tests

Visual Basic only

Microsoft Reviewers: Open in CodeFlow

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 88.18824% with 251 lines in your changes missing coverage. Please review.

Project coverage is 74.76345%. Comparing base (88d7659) to head (54ea3f5). Report is 1 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11215         +/-   ##
===================================================
+ Coverage   74.57429%   74.76345%   +0.18916%     
===================================================
  Files           3041        3056         +15     
  Lines         629823      631485       +1662     
  Branches       46846       46877         +31     
===================================================
+ Hits          469686      472120       +2434     
+ Misses        156783      155997        -786     
- Partials        3354        3368         +14     

Flag	Coverage Δ
Debug	74.76345% <88.18824%> (+0.18916%)	:arrow_up:
integration	17.96623% <14.77273%> (+0.08492%)	:arrow_up:
production	47.77523% <62.27273%> (+0.30010%)	:arrow_up:
test	96.96834% <94.95549%> (-0.01002%)	:arrow_down:
unit	44.83671% <52.04545%> (+0.27165%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

[paul1956]

@KlausLoeffelmann can you get someone assigned to review please. This consolidates much of the cleanup and adds testing to VB code. Some files are not touched as they require code changes covered in other PR's.

[lonitra]

Appreciate your patience here @paul1956! @KlausLoeffelmann is preparing for BUILD talk this week, but will take a look at your PRs afterwards

[paul1956]

Ping @lonitra @KlausLoeffelmann @JeremyKuhne Any update on Review timing?

[KlausLoeffelmann]

We just finished BUILD and some immediate security issues and need to regroup now. I have something VB related coming up in this context in the next days, which is a higher priority. I'll ping you with it once I have a draft.

[elachlan]

@paul1956 good work here. A lot of the xml comments need punctuation added (full stops).

A lot of this could have been split up into multiple PRs to make it easier to land.

[paul1956]

@elachlan there are numerous places where <see cref="Something"/> could be used what are the rules because it would touch most functions.

[paul1956]

The cleanup of XML comments should have been part of the code analysis and code fixes, it is a waste of human time and it doesn’t improve  the code which has been mostly unchanged for a decade, until the punctuation I was able to automate most of it at this, point it’s reading every comment. All the tests improve the code and prevent breaking changes when removing WebClient and extending VB IO to be async.I will keep doing it but wanted to express my frustration with the process and changing the bar.  Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Jun 21, 2024, at 7:20 PM, Lachlan Ennis @.***> wrote: @paul1956 good work here. A lot of the xml comments need punctuation added (full stops). A lot of this could have been split up into multiple PRs to make it easier to land.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[elachlan]

I'll do my best to help you get this merged.

[paul1956]

I'll do my best to help you get this merged.

I would like to sort functions, privates used rules and a tool. Should that go into a follow-on PR after Merge, or as part of fixing removal of WebClient or somewhere else. Would it be best to do 1 file per PR or some other strategy. All the new code is already sorted and has clean XML Comments.

[elachlan]

You are much more likely to get a PR merged if its smaller. Large change sets like this are very hard to review properly unless its a singular change like applying a code fix.

Do not add more stuff to this PR, it really should have been split up into several PRs already. Reordering can be done in another PR after this is merged.

If we can't get any movement on getting this merged, we can split it up into smaller PRs. That will allow the team to quickly review and okay changes.

[paul1956]

@elachlan thanks, I noticed the translation files have ". " Throughout should I open an issue?

Latest merge from Master caused 80+ new issues and existing suppressions are not working, or names don't match new issues. Most of them are around UnsafeNativeMethods and NativeMethods that I don't want to remove in case someone wants to replace them with "shared primitives project and cswin32". Should I open issue?

[elachlan]

My comment about cswin32 is more for the winforms team. I think last time I mentioned it there were concerns raised. But I can't remember much.

[elachlan]

when you are merging master, are you rebasing? There are a few files that have changed in the PR which are irrelevant to the actual PR. global.json, Version.props, Version.Details.xml.

[paul1956]

when you are merging master, are you rebasing? There are a few files that have changed in the PR which are irrelevant to the actual PR. global.json, Version.props, Version.Details.xml.

That was an error, I thought I reverted the 2 commits. I could not find any way to cancel the commits accidentally to main. Web to the rescue and I was able to undo the merge into master,

@elachlan please let me know if there are still issues with Main/master. All my changes should only be to my PR.

[elachlan]

In visual basic the namespace is made up of the rootnamespace in the project file plus the namespace inside the file.

[paul1956]

In visual basic the namespace is made up of the rootnamespace in the project file plus the namespace inside the file.

Yes I was actually at Master where it’s incorrect. VS was slow in updating Test Explorer when I switched branches.

[paul1956]

@lonitra @KlausLoeffelmann I am going to break this PR into very small parts to simplify review and merge. #11653 is the first with just 3 files changed.

[elachlan]

@paul1956 I am not on the winforms team (or with microsoft/dotnet), so I have no control over when stuff is reviewed or merged.

I'd say it would be easier to split this PR into smaller PRs. Can the test changes be moved to their own PR?

The project template changes can be done in a small PR that would probably be merged immediately.

Are the .gitattributes changes on purpose? That looks like it reverts changes that were made in a PR recently.

[paul1956]

@elachlan I created PR #11653 which is the first part of this larger PR. I will break it down in multiple parts to simplify review. Thanks for the feedback

[paul1956]

@lonitra @KlausLoeffelmann Everything in here has been merger into Main or #11655 so I am closing