Skip to content

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 17, 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.

Description

Obsolete every constructor on Response besides the one which sets all values. Obsolete public setters on Response properties.

Motivation and Context

A programmatic guarantee of immutability would be a conceptual simplification, because it allows us to make assumptions and guarantees we could not otherwise make.

For example, in the future we could expose JsonNode? ValueAsNode property which would allow us to manage the response as a strongly-typed node instead of knowing ahead of time what the types could be.

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, Other


Description

  • Marked constructors and setters in Response as obsolete.

  • Introduced warnings for deprecated Response members.

  • Updated HttpCommandExecutor to handle obsolete Response.Value setter.

  • Prepared Response class for immutability in future releases.


Changes walkthrough 📝

Relevant files
Enhancement
HttpCommandExecutor.cs
Suppress warnings for obsolete `Response.Value` setter     

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs

  • Added pragma directives to suppress warnings for obsolete
    Response.Value setter.
  • Adjusted CreateResponse method to handle deprecated Response.Value.
  • +2/-0     
    Response.cs
    Mark `Response` constructors and setters as obsolete         

    dotnet/src/webdriver/Response.cs

  • Marked default and single-parameter constructors as obsolete.
  • Marked setters for Value, SessionId, and Status as obsolete.
  • Added warnings for deprecated members to prepare for immutability.
  • +22/-3   

    Need help?
  • Type /help how to ... in the comments thread for any question 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 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Breaking Change

    The obsolete warning message indicates removal in Selenium 4.30, but this change could break existing code that relies on the mutable properties. Consider if a longer deprecation timeline is needed.

    [Obsolete("Set all values using the Response(string, object, WebDriverResult) constructor instead. This constructor will be removed in Selenium 4.30")] public Response()
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 17, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Replace direct property modification with creation of new immutable instance to maintain object immutability
    Suggestion Impact:The commit implemented the suggestion by replacing the direct Value property modification with creation of a new Response instance, maintaining immutability

    code diff:

    -#pragma warning disable CS0618 // Response.Value setter can be used internally - response.Value = valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine); -#pragma warning restore CS0618 + valueString = valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine); + response = new Response(response.SessionId, valueString, response.Status);

    Instead of using the deprecated Value setter, create a new Response instance with
    the modified value to maintain immutability principles.

    dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [344]

    -response.Value = valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine); +response = new Response(response.SessionId, valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine), response.Status);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion provides a better approach to handle the deprecated Value setter by creating a new immutable Response instance, which aligns with the PR's goal of making Response immutable. This is a significant improvement in terms of design and future compatibility.

    8
    Use init-only setters to enforce immutability while maintaining initialization flexibility

    Consider using the init accessor instead of deprecated setters to enforce
    immutability while still allowing object initialization.

    dotnet/src/webdriver/Response.cs [141-147]

    -public object? Value -{ - get; - [Obsolete("The Response type will be immutable and this setter will be removed in Selenium 4.30")] - set; -} +public object? Value { get; init; }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using init-only setters is a modern C# feature that would effectively enforce immutability while still allowing object initialization. This suggestion aligns well with the PR's goal of making Response immutable and provides a cleaner alternative to the current approach with Obsolete attributes.

    7
    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thx, it is way to improve.

    @RenderMichael RenderMichael merged commit 3358983 into SeleniumHQ:trunk Jan 17, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the response branch January 17, 2025 22:40
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    … conducive to immutability (SeleniumHQ#15107) * [dotnet] Obsoletes setters on `Response` type * [dotnet] Obsolete constructors on `Response` that are not conducive to immutability * Avoid obsolete `Response` constructor
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    … conducive to immutability (SeleniumHQ#15107) * [dotnet] Obsoletes setters on `Response` type * [dotnet] Obsolete constructors on `Response` that are not conducive to immutability * Avoid obsolete `Response` constructor
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    2 participants