winforms icon indicating copy to clipboard operation
winforms copied to clipboard

Vb code cleanup

Open paul1956 opened this issue 1 year ago • 12 comments

Fixes #

Proposed changes

Clean up Visual Basic Code to eliminate suggestions, fixes 85 issues flagged by Visual Studio

Customer Impact

None, if targets developers who get 85 issues that clutter up the Error List

Regression?

No, it more about making code match .editorConfig included with project. Alternativity, .editorConfig could be changed to match the Visual Basic Code but that will cause other cosmetic issues, where other code does not match newer .editorConfig.

Risk

Very low, mostly renaming of local variables, fix spelling errors in comments and adding missing "Public" on public functions, biggest change is If Statement to if Expression.

Test methodology

Code completes, existing tests pass, changes are mostly cosmetic.

This is only in 1 Project written in old Visual Basic

Microsoft Reviewers: Open in CodeFlow

paul1956 avatar Oct 14 '23 10:10 paul1956

I would be surprised, if the Roslyn team doesn't have a dedicated .editorConfig for VB. Couldn't we steal that from them? I cannot believe that the current setting we're using should be some sort of standard for VB.

@AlekseyTs: Asking for a friend! 😸

KlausLoeffelmann avatar Oct 18 '23 00:10 KlausLoeffelmann

Sorry. I prefer to stay oblivious of the .editorConfig stuff. Feel free to check what we have in the Roslyn repo. I personally never reformat entire file, and I would revert spacing changes if VS were to make them without me explicitly looking to make the changes. Regardless of what .editorConfig has.

AlekseyTs avatar Oct 18 '23 00:10 AlekseyTs

I would be surprised, if the Roslyn team doesn't have a dedicated .editorConfig for VB. Couldn't we steal that from them? I cannot believe that the current setting we're using should be some sort of standard for VB.

@AlekseyTs: Asking for a friend! 😸

@KlausLoeffelmann Roslyn has 1 editorConfig for the solution (VB and C#), a few ancillary repos have their own which inherit from the root one.

Spacing is one issue where blank lines after classes, punctions is currently specified and saving applies the formatting. it's possible there is a VS Setting to prevent this the auto formating but I haven't found it.

A separate issue is preferring If Expressions for C# and If Statements for VB. This is controlled by .editorConfig and right now they share the same settings to prefer If Expressions.

paul1956 avatar Oct 18 '23 02:10 paul1956

@KlausLoeffelmann I have manually removed all the extra lines that VS wants to add, the biggest manual change is separating Imports by root type. It that is a setting I don't know how to turn it off.

I have added ExceptionUtils and other prefixes like it back. The code was not consistent in how/when it used ExceptionUtils directly vs in an Imports. I have made all uses follow below.

Imports ExUtils = Microsoft.VisualBasic.CompilerServices.ExceptionUtils

paul1956 avatar Oct 19 '23 00:10 paul1956

@KlausLoeffelmann Ping This is all cleaned up with updated and separated editorConfig and updated VB project file with options specified. I think I removed all the extra blank lines, but any editing in VS will bring them back. Someone that knows about controlling the blank lines should review and recommend a fix that is more automatic and not requiring removing the blanks using Notepad. This should be reviewed and merged before the Fix to remove WebClient which has all the same formatting issues.

FYI there is no consistency on use of blank lines before or after End Class and after End NameSpace or after File header in original source.

paul1956 avatar Oct 24 '23 09:10 paul1956

I need to take a bit more time, which I don't have right now. I'll dial back to this in a bit. Sorry for the delay!

KlausLoeffelmann avatar Oct 25 '23 00:10 KlausLoeffelmann

I need to take a bit more time, which I don't have right now. I'll dial back to this in a bit. Sorry for the delay!

@KlausLoeffelmann no issue, I continue to fix formatting differences between different files. I will continue to keep this PR up to date with Main.

paul1956 avatar Oct 26 '23 00:10 paul1956

When you get the chance, can you also start using the name "main" for the equivalence of our main branch to avoid confusion? Thx!

KlausLoeffelmann avatar Oct 26 '23 06:10 KlausLoeffelmann

@KlausLoeffelmann gentle ping to keep this on your radar to take this to completion

lonitra avatar Nov 16 '23 18:11 lonitra

Yes, Ma'am. As soon as I can get to it! Give me a bit more time - thanks for pinging me!

KlausLoeffelmann avatar Nov 16 '23 18:11 KlausLoeffelmann

Ping, is there an ETA when someone might review this?

paul1956 avatar Jan 09 '24 01:01 paul1956

Codecov Report

Attention: Patch coverage is 81.48496% with 197 lines in your changes are missing coverage. Please review.

Project coverage is 73.31195%. Comparing base (fd95fcd) to head (24e5ab5).

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #10117         +/-   ##
===================================================
+ Coverage   73.22354%   73.31195%   +0.08840%     
===================================================
  Files           3096        3104          +8     
  Lines         633620      634254        +634     
  Branches       47336       47341          +5     
===================================================
+ Hits          463959      464984       +1025     
+ Misses        166112      165730        -382     
+ Partials        3549        3540          -9     
Flag Coverage Δ
Debug 73.31195% <81.48496%> (+0.08840%) :arrow_up:
integration 18.41764% <1.45985%> (-0.00230%) :arrow_down:
production 46.84488% <52.06813%> (+0.13442%) :arrow_up:
test 94.98804% <100.00000%> (+0.00853%) :arrow_up:
unit 43.74090% <50.85158%> (+0.12442%) :arrow_up:

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

codecov[bot] avatar Jan 11 '24 00:01 codecov[bot]

This is replace by other PR's and is unnecessary

paul1956 avatar Mar 18 '24 00:03 paul1956