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

Code cleanup only and Add VB Tests #11215

Open
wants to merge 130 commits into
base: main
Choose a base branch
from

Conversation

paul1956
Copy link
Contributor

@paul1956 paul1956 commented Apr 14, 2024

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

@paul1956 paul1956 requested a review from a team as a code owner April 14, 2024 22:43
Copy link

codecov bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 91.56315% with 163 lines in your changes missing coverage. Please review.

Project coverage is 74.59650%. Comparing base (5ba64ae) to head (6fd30f9).

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #11215         +/-   ##
===================================================
+ Coverage   74.43217%   74.59650%   +0.16433%     
===================================================
  Files           3039        3054         +15     
  Lines         629056      630539       +1483     
  Branches       46834       46865         +31     
===================================================
+ Hits          468220      470360       +2140     
+ Misses        157483      156830        -653     
+ Partials        3353        3349          -4     
Flag Coverage Δ
Debug 74.59650% <91.56315%> (+0.16433%) ⬆️
integration 17.90302% <15.22248%> (+0.00827%) ⬆️
production 47.48589% <62.99766%> (+0.23132%) ⬆️
test 96.96458% <99.66777%> (+0.01194%) ⬆️
unit 44.55098% <52.45902%> (+0.24920%) ⬆️

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

@paul1956
Copy link
Contributor Author

@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.

@elachlan
Copy link
Contributor

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
Copy link
Contributor Author

@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?

paul1956 and others added 4 commits June 22, 2024 16:43
…icationServices/WindowsFormsApplicationBase.vb

Co-authored-by: Lachlan Ennis <2433737+elachlan@users.noreply.github.com>
…icationServices/WindowsFormsApplicationBase.vb

Co-authored-by: Lachlan Ennis <2433737+elachlan@users.noreply.github.com>
@elachlan
Copy link
Contributor

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.

paul1956 and others added 14 commits June 22, 2024 17:31
Add [CollectionDefinition("Sequential",DisableParallelization =true)] which prevents Clipboard tests from funning in parrallel with ANY othe collections.
Split ReferencedStream to dedicated file
…icationServices/WindowsFormsApplicationBase.vb


Add period

Co-authored-by: Lachlan Ennis <2433737+elachlan@users.noreply.github.com>
This reverts commit 3c20702.
@elachlan
Copy link
Contributor

elachlan commented Jun 24, 2024

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
Copy link
Contributor Author

paul1956 commented Jun 25, 2024

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
Copy link
Contributor

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

@paul1956
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-review This item is waiting on review by one or more members of team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants