Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

paul1956 · 2024-09-11T08:49:52Z

Fixes VB Long lines

Proposed changes

shorten long lines in VB

Customer Impact

None

Regression?

No

Risk

-Minimal No code changes except formatting

Microsoft Reviewers: Open in CodeFlow

codecov · 2024-09-11T09:23:54Z

Codecov Report

Attention: Patch coverage is 81.87221% with 488 lines in your changes missing coverage. Please review.

Project coverage is 75.87729%. Comparing base (441fb2a) to head (e3520d4).
Report is 6 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #12118         +/-   ##
===================================================
+ Coverage   75.74060%   75.87729%   +0.13669%     
===================================================
  Files           3157        3163          +6     
  Lines         636104      637876       +1772     
  Branches       47003       47059         +56     
===================================================
+ Hits          481789      484003       +2214     
+ Misses        150854      150418        -436     
+ Partials        3461        3455          -6

Flag	Coverage Δ
Debug	`75.87729% <81.87221%> (+0.13669%)`	⬆️
integration	`18.15941% <7.79854%> (-0.01551%)`	⬇️
production	`49.52664% <60.43867%> (+0.21988%)`	⬆️
test	`97.05740% <99.93155%> (+0.01205%)`	⬆️
unit	`46.72270% <54.99594%> (+0.20340%)`	⬆️

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

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

...otnet.WinForms.ProjectTemplates/content/WinFormsApplication-VisualBasic/ApplicationEvents.vb

...Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkAvailability.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkDownload.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkUpload.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/ClipboardProxy.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/FileSystemProxy.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/Internal/HttpClientCopy.vb

src/Microsoft.VisualBasic.Forms/tests/UnitTests/System/Windows/Forms/NetworkDownloadTests.vb

Tanya-Solyanik

@paul1956 - is this PR ready for a review, or are you still waiting to merge previous PRs? I would suggest factoring out parts of this PR that are not based on the previous changes and are dedicated only to comments and long code lines.

...ft.VisualBasic.Forms/src/Microsoft/VisualBasic/ApplicationServices/ConsoleApplicationBase.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Clock.vb

Tanya-Solyanik · 2024-09-13T17:30:09Z

...Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkAvailability.vb

+
+Imports NetInfoAlias = System.Net.NetworkInformation
+
+Namespace Microsoft.VisualBasic.Devices


Why did you move this file, the namespace matches Microsoft\VisualBasic\Devices path. And this directory contains only 11 items, so it is manageable. I don't see a reason for this move.

Previous Feedback to move classes into their own file

Into their own file, not into a new folder.

@Tanya-Solyanik The entire focus of all the VB PR's was to fix the networking code. Putting it all together in a directory just make it simpler for me. Originally it was all in 1 file. I have moved them back.

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Helpers/SafeNativeMethods.vb

Tanya-Solyanik · 2024-09-13T17:39:17Z

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/Internal/HttpClientCopy.vb

+Imports System.Threading
+Imports Microsoft.VisualBasic.Devices.NetworkUtilities
+
+Namespace Microsoft.VisualBasic.MyServices.Internal


Could you please pull out this refactoring from this PR into a separate one? It will be easier to review.

@Tanya-Solyanik It is, this PR should be reviewed in order then only 6 files core to the issue are changed, httpClientCopy is the whole purpose of this PR. I have changed everything to DRAFT except 1.

Sorry my lack of knowledge of PR process has caused you to do large reviews and almost null reviews.

Everything I have done is to get to this PR which has the critical fix as webClient is going to be made obsolete.

Did you factor out "critical fix as webClient"?

Did you factor out "critical fix as webClient"?

@Tanya-Solyanik the Critical fix was to replace WebClient with HttpClient for both network upload and download.

WebCient was split into two parts the Upload part is currently unchanged because I have no acceptable way to test without 3rd party code/server (my prototype used public servers to test existing code). I created my own download server in the repo so I can test download and replaced WebClient with HttpClient for download only.

We shouldn't test WebClient or HttpClient because we don't own them.

@Tanya-Solyanik the replacement needs to return the identical error codes to not be a breaking change. I am not testing WebClient or HttpClient by themselves just as they are used in NetworkDownload.

Tanya-Solyanik · 2024-09-13T17:40:27Z

src/Microsoft.VisualBasic.Forms/src/Properties/AssemblyInfo.vb

+
+Imports System.Runtime.CompilerServices
+
+<Assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")>


Is this coming from another PR?

Yes one being discussed by @LeafShi1 and @KlausLoeffelmann

ricardobossan · 2024-09-16T10:13:17Z

src/System.Windows.Forms/src/System/Windows/Forms/Input/InputLanguage.cs

@@ -109,7 +109,8 @@ public string LayoutName
    }

    /// <summary>
-    ///  Returns the <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew">
+    ///  Returns the
+    ///  <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew">
    ///  keyboard layout identifier</see> of the current input language.


I think this better follows the pattern of the other see tags in the source code:

/// <summary> /// Returns the /// <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew"> /// keyboard layout identifier /// </see> /// of the current input language. /// </summary>

@ricardobossan good feedback but why are you reviewing a draft PR? All the draft PRs depend on the previous ones. So there would be a tremendous amounts of duplicate work. There are 3 that need to merge before this one.

Because there were comments being made already. But I'll wait out next time.

@ricardobossan sorry that was my fault I did not understand the review process and only learned about Draft Friday.

There are several more I did without Draft that could be reviewed but I will need a little time at apply this comment to them.

Thanks for your understanding.

@ricardobossan opened a new PR #12151 based on main to just deal with this issue, there are just a few places where it does not follow the pattern. I will address expected merge conflicts in future PR's where they occur.

…-3rd

…-after-FixIssue#9807-3rd

[main] Update dependencies from dotnet/arcade

* Add code coverage for TrackBarRenderer * Handle FeedBacks * Handle FeedBacks * Handle FeedBacks * Handle FeedBacks

[main] Update dependencies from dotnet/arcade

It doesn't really matter anymore and is creating extra maintenance cost as we add new APIs. Remove versioning.targets as it did very little and all over the override attributes. See a discussion on this here: dotnet/runtime#44194

…ue#9807-2nd

…-after-FixIssue#9807-3rd

…07-2nd-DRAFT

paul1956 requested a review from a team as a code owner September 11, 2024 08:49

dotnet-policy-service bot assigned paul1956 Sep 11, 2024

Tanya-Solyanik requested review from LeafShi1, SimonZhao888, Epica3055 and ricardobossan September 11, 2024 21:30

Tanya-Solyanik added waiting-review This item is waiting on review by one or more members of team 📭 waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Sep 11, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 11, 2024

paul1956 changed the title ~~Shorten long lines in VB review after fix issue#9807~~ Shorten long lines in VB REVIEW AFTER fix issue#9807 Sep 12, 2024

LeafShi1 reviewed Sep 12, 2024

View reviewed changes

LeafShi1 added 📭 waiting-author-feedback The team requires more information from the author a11y-external An accessibility bug that is external to WinForms (for example, a Common Dialog in Win32) labels Sep 12, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 12, 2024

Tanya-Solyanik added 📭 waiting-author-feedback The team requires more information from the author and removed a11y-external An accessibility bug that is external to WinForms (for example, a Common Dialog in Win32) labels Sep 12, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

Tanya-Solyanik reviewed Sep 13, 2024

View reviewed changes

Tanya-Solyanik added the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

paul1956 changed the title ~~Shorten long lines in VB REVIEW AFTER fix issue#9807~~ Shorten long lines in VB REVIEW AFTER fix issue#9807 DRAFT Sep 13, 2024

Tanya-Solyanik added the draft draft PR label Sep 14, 2024

Tanya-Solyanik marked this pull request as draft September 14, 2024 00:23

paul1956 changed the title ~~Shorten long lines in VB REVIEW AFTER fix issue#9807 DRAFT~~ Shorten long lines in VB REVIEW (4th) AFTER fix issue#9807 DRAFT Sep 16, 2024

ricardobossan reviewed Sep 16, 2024

View reviewed changes

paul1956 added 3 commits September 17, 2024 21:00

Merge branch 'Add-Tests-for-VB-Runtime-Review-2nd' into FixIssue#9807…

89cf072

…-3rd

Merge

d21e9af

Merge Master

d5c5e0a

paul1956 and others added 21 commits November 11, 2024 02:12

Merge branch 'FixIssue#9807-2nd' into Shorten-Long-Lines-in-VB-Review…

bc21866

…-after-FixIssue#9807-3rd

[main] Update dependencies from dotnet/arcade (dotnet#12459)

df346b8

[main] Update dependencies from dotnet/arcade

Add code coverage for TrackBarRenderer (dotnet#12427)

0bbdead

* Add code coverage for TrackBarRenderer * Handle FeedBacks * Handle FeedBacks * Handle FeedBacks * Handle FeedBacks

[main] Update dependencies from dotnet/arcade (dotnet#12464)

2331748

[main] Update dependencies from dotnet/arcade

Remove CLSCompliant attribute (dotnet#12469)

4ad9cdf

It doesn't really matter anymore and is creating extra maintenance cost as we add new APIs. Remove versioning.targets as it did very little and all over the override attributes. See a discussion on this here: dotnet/runtime#44194

Delete build.sh (dotnet#12465)

4361014

Merge branch 'Add-Network-Download-Tests-Only-Review-1st' into FixIss…

d869cf1

…ue#9807-2nd

Merge branch 'FixIssue#9807-2nd' into Shorten-Long-Lines-in-VB-Review…

7653c69

…-after-FixIssue#9807-3rd

Add NetwrokDownloadTests and WebListner

1cd54bf

Fix merge issues

66298ab

Sync changes from PR dotnet#11867

08be576

Simplify code

24551f1

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

bf912fe

Merge branch 'master' into FixIssue#9807-2nd-DRAFT

f741607

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

ea1d00f

…07-2nd-DRAFT

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

1adf2be

Merge branch 'master' into FixIssue#9807-2nd-DRAFT

fae6727

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

af840f2

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

933b97d

…07-2nd-DRAFT

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

8f280c4

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

b5da52c

…07-2nd-DRAFT

paul1956 changed the title ~~Shorten long lines in VB REVIEW (3rd) AFTER fix issue#9807 DRAFT~~ Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT Nov 22, 2024

paul1956 added 8 commits November 21, 2024 20:37

Minor Cleanup

bf91442

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

d1b4234

Merge branch 'master' into FixIssue#9807-2nd-DRAFT

f3423d4

Merge

340c744

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

4e48605

Merge branch 'master' into FixIssue#9807-2nd-DRAFT

1f2e39d

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

c687aa1

…07-2nd-DRAFT

Merge

e3520d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

paul1956 commented Sep 11, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

Tanya-Solyanik left a comment

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 13, 2024

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 14, 2024 •

edited

Loading

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 14, 2024

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 14, 2024 •

edited

Loading

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 15, 2024

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 14, 2024

ricardobossan Sep 16, 2024

paul1956 Sep 16, 2024

ricardobossan Sep 16, 2024

paul1956 Sep 16, 2024

paul1956 Sep 16, 2024


		Imports NetInfoAlias = System.Net.NetworkInformation

		Namespace Microsoft.VisualBasic.Devices


		Imports System.Runtime.CompilerServices

		<Assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")>

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Are you sure you want to change the base?

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Conversation

paul1956 commented Sep 11, 2024 • edited by dotnet-policy-service bot Loading

Proposed changes

Customer Impact

Regression?

Risk

Microsoft Reviewers: Open in CodeFlow

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 commented Sep 11, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

paul1956 Sep 14, 2024 •

edited

Loading

paul1956 Sep 14, 2024 •

edited

Loading