-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[dotnet] Address nullable warnings #15389
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
base: trunk
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
@@ -191,7 +191,7 @@ public override async Task ContinueRequestWithResponse(HttpRequestData requestDa | |||
|
|||
var commandSettings = new FulfillRequestCommandSettings() | |||
{ | |||
RequestId = requestData.RequestId, | |||
RequestId = requestData.RequestId!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance requestData.RequestId
should not be nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestId
can be null in one specific circumstance: when a user creates an HttpRequestData
themselves.
This only happens in one place, and it can even cause problems. I filed an issue about it: #15380
However, these HttpRequestData
s are known to be created internally, so we know RequestId
is not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, so requestData.RequestId
should not be null. Then I propose to not suppress this in this PR, because of we will definitely forget re-suppress it. Let's try to fix root cause, even if it will be a breaking change.
What I understand, we have 2 ways:
- Slightly adjust public API to require
RequestID
to be not nullable (following standard policy of deprecation), will be a part ofv4
- Entirely revisit public API of
NetworkManager
inv5
(how this API looks, how it could look, including renamingStartMonitoring
toStartMonitoringAsync
)
@RenderMichael if can manage the 1st way easily with minimal effort, it would be great (as short-term solution before v5
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a much better solution.
All 3 proposed solutions fix the issue (2 is my favorite but breaking, 3 is still good, 1 is the backup which is not a breaking change).
User description
Description
This PR addresses lingering nullability warnings which can be fixed trivially.
Remaining work is contained in #15380 and #15379
Motivation and Context
Types of changes
Checklist
PR Type
Bug fix
Description
Addressed nullable warnings in multiple
Network
classes.Added null-forgiving operator (
!
) toRequestId
assignments.Updated
WebDriver
class to makeNetworkManager
nullable.Improved code safety by handling potential nullability issues.
Changes walkthrough 📝
V131Network.cs
Fix nullable warnings in V131Network class
dotnet/src/webdriver/DevTools/v131/V131Network.cs
!
) toRequestId
assignments.ContinueRequest
andAddResponseBody
methods.V132Network.cs
Fix nullable warnings in V132Network class
dotnet/src/webdriver/DevTools/v132/V132Network.cs
!
) toRequestId
assignments.ContinueRequest
andAddResponseBody
methods.V133Network.cs
Fix nullable warnings in V133Network class
dotnet/src/webdriver/DevTools/v133/V133Network.cs
!
) toRequestId
assignments.ContinueRequest
andAddResponseBody
methods.V85Network.cs
Fix nullable warnings in V85Network class
dotnet/src/webdriver/DevTools/v85/V85Network.cs
!
) toRequestId
assignments.ContinueRequest
andAddResponseBody
methods.WebDriver.cs
Update WebDriver class for nullable `NetworkManager`
dotnet/src/webdriver/WebDriver.cs
NetworkManager
field nullable.