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

[dotnet] Simplify user creation of network types #15267

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 10, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Simplify user creation of network types. Do not require any constructors.

Motivation and Context

Fix a break I introduced by adding constructors for HTTP networking types. I did not think these types were intended to be created by users; they were.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests


Description

  • Simplified creation of HttpRequestData and HttpResponseData objects by removing constructors.

  • Updated network-related classes to use object initializers for better readability.

  • Adjusted properties in HttpRequestData and HttpResponseData to allow internal and nullable setters.

  • Added a new test for transforming network responses in NetworkInterceptionTests.


Changes walkthrough 📝

Relevant files
Enhancement
8 files
V130Network.cs
Refactored network event handling to use object initializers
+15/-15 
V131Network.cs
Refactored network event handling to use object initializers
+15/-15 
V132Network.cs
Refactored network event handling to use object initializers
+15/-15 
V85Network.cs
Refactored network event handling to use object initializers
+14/-14 
HttpRequestData.cs
Removed constructor and updated properties for flexibility
+1/-10   
HttpResponseData.cs
Removed constructor and updated properties for flexibility
+4/-14   
NetworkManager.cs
Adjusted `HttpRequestData` usage to use object initializer
+1/-1     
NetworkResponseReceivedEventArgs.cs
Made properties nullable for better flexibility                   
+3/-3     
Tests
1 files
NetworkInterceptionTests.cs
Added test for transforming network responses                       
+27/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication

    The comment about creating a dummy HttpRequestData object suggests this is a code smell that could be refactored to avoid the need for creating dummy objects. Consider refactoring the ContinueRequestWithResponse method to accept just the request ID or response data.

    // NOTE: We create a dummy HttpRequestData object here, because the ContinueRequestWithResponse
    // method demands one; however, the only property used by that method is the RequestId property.
    // It might be better to refactor that method signature to simply pass the request ID, or
    // alternatively, just pass the response data, which should also contain the request ID anyway.
    HttpRequestData requestData = new HttpRequestData { RequestId = e.ResponseData.RequestId };
    await this.session.Value.Domains.Network.ContinueRequestWithResponse(requestData, handler.ResponseTransformer(e.ResponseData)).ConfigureAwait(false);

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Add null parameter validation at the start of methods to prevent NullReferenceExceptions

    The OnFetchRequestPaused method accepts a nullable sender and e parameters but
    doesn't validate them before use. Add null checks at the start of the method to
    prevent potential NullReferenceExceptions, especially since e and its properties
    are used extensively.

    dotnet/src/webdriver/DevTools/v130/V130Network.cs [338-342]

     private void OnFetchRequestPaused(object? sender, Fetch.RequestPausedEventArgs e)
     {
    +    ArgumentNullException.ThrowIfNull(e, nameof(e));
    +    ArgumentNullException.ThrowIfNull(e.Request, nameof(e.Request));
    +    
         if (e.ResponseErrorReason == null && e.ResponseStatusCode == null)
         {
             var requestData = new HttpRequestData

    [To ensure code accuracy, apply this suggestion manually]

    Low
    General
    Remove redundant constructor parentheses

    Remove the unnecessary empty parentheses after HttpResponseData instantiation to
    maintain consistent object initialization style.

    dotnet/src/webdriver/DevTools/v130/V130Network.cs [354-361]

    -var responseData = new HttpResponseData()
    +var responseData = new HttpResponseData
     {
         RequestId = e.RequestId,
         Url = e.Request.Url,
         ResourceType = e.ResourceType.ToString(),
         StatusCode = e.ResponseStatusCode.GetValueOrDefault(),
         ErrorReason = e.ResponseErrorReason?.ToString()
     };
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion improves code style consistency by removing unnecessary empty parentheses in object initialization, but has minimal impact on functionality or maintainability.

    Low

    @RenderMichael RenderMichael merged commit 0b98c78 into SeleniumHQ:trunk Feb 10, 2025
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the http-request-data-2 branch February 10, 2025 22:10
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    * [dotnet] Simplify user creation of network types
    
    * add test
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant