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] [bidi] Make LocalValue types not nested #15428

Merged
merged 5 commits into from
Mar 16, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Mar 15, 2025

User description

Motivation and Context

Contributes to #15407

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

  • Refactored LocalValue nested types to standalone types for better extensibility.

  • Updated all references to LocalValue nested types in codebase and tests.

  • Improved type naming consistency by appending LocalValue to standalone types.

  • Enhanced test coverage to validate changes in LocalValue type structure.


Changes walkthrough 📝

Relevant files
Enhancement
3 files
BiDiJsonSerializerContext.cs
Updated JSON serialization to use standalone `LocalValue` types.
+1/-2     
AddPreloadScriptCommand.cs
Refactored AddPreloadScriptCommand to use standalone LocalValue types.
+3/-3     
LocalValue.cs
Converted `LocalValue` nested types to standalone types. 
+44/-43 
Tests
4 files
CallFunctionLocalValueTest.cs
Updated tests to validate standalone `LocalValue` types. 
+18/-18 
CallFunctionParameterTest.cs
Adjusted tests for `LocalValue` type refactoring.               
+1/-1     
ScriptCommandsTest.cs
Modified tests to use refactored `LocalValue` types.         
+2/-2     
ScriptEventsTest.cs
Updated event tests to reflect `LocalValue` type changes.
+1/-1     

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:

    🎫 Ticket compliance analysis ✅

    15407 - Fully compliant

    Compliant requirements:

    • Refactored nested DTO types in the .NET BiDi implementation to standalone types
    • Moved away from the pattern of using nested types to standalone types
    • Updated all types with similar pattern
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Naming Consistency

    The PR appends 'LocalValue' suffix to all types derived from LocalValue. This is good for consistency but might be redundant in some contexts. Consider if this naming convention is the best approach for the codebase.

    [JsonDerivedType(typeof(NumberLocalValue), "number")]
    [JsonDerivedType(typeof(StringLocalValue), "string")]
    [JsonDerivedType(typeof(NullLocalValue), "null")]
    [JsonDerivedType(typeof(UndefinedLocalValue), "undefined")]
    [JsonDerivedType(typeof(ChannelLocalValue), "channel")]
    [JsonDerivedType(typeof(ArrayLocalValue), "array")]
    [JsonDerivedType(typeof(DateLocalValue), "date")]
    [JsonDerivedType(typeof(MapLocalValue), "map")]
    [JsonDerivedType(typeof(ObjectLocalValue), "object")]
    [JsonDerivedType(typeof(RegExpLocalValue), "regexp")]
    [JsonDerivedType(typeof(SetLocalValue), "set")]
    Code Organization

    The refactoring moved nested types to standalone types but kept them in the same file. Consider if these types should be split into separate files for better maintainability.

    public abstract record PrimitiveProtocolLocalValue : LocalValue;
    
    public record NumberLocalValue(double Value) : PrimitiveProtocolLocalValue
    {
        public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
    }
    
    public record StringLocalValue(string Value) : PrimitiveProtocolLocalValue;
    
    public record NullLocalValue : PrimitiveProtocolLocalValue;
    
    public record UndefinedLocalValue : PrimitiveProtocolLocalValue;
    
    public record ChannelLocalValue(ChannelLocalValue.ChannelProperties Value) : LocalValue
    {
        // TODO: Revise why we need it
        [JsonInclude]
        internal string type = "channel";
    
        public record ChannelProperties(Channel Channel)
        {
            public SerializationOptions? SerializationOptions { get; set; }
    
            public ResultOwnership? Ownership { get; set; }
        }
    }
    
    public record ArrayLocalValue(IEnumerable<LocalValue> Value) : LocalValue;
    
    public record DateLocalValue(string Value) : LocalValue;
    
    public record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue;
    
    public record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue;
    
    public record RegExpLocalValue(RegExpLocalValue.RegExpValue Value) : LocalValue
    {
        public record RegExpValue(string Pattern)
        {
            public string? Flags { get; set; }
        }
    }
    
    public record SetLocalValue(IEnumerable<LocalValue> Value) : LocalValue;

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 15, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove redundant type field

    The type field is redundant and potentially problematic since the type is
    already specified through the [JsonDerivedType] attribute with "channel" as the
    discriminator. The comment "TODO: Revise why we need it" suggests this is
    already recognized as an issue. Remove this field to avoid potential
    serialization conflicts.

    dotnet/src/webdriver/BiDi/Modules/Script/LocalValue.cs [87-98]

     public record ChannelLocalValue(ChannelLocalValue.ChannelProperties Value) : LocalValue
     {
    -    // TODO: Revise why we need it
    -    [JsonInclude]
    -    internal string type = "channel";
    -
         public record ChannelProperties(Channel Channel)
         {
             public SerializationOptions? SerializationOptions { get; set; }
     
             public ResultOwnership? Ownership { get; set; }
         }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a redundant field that could cause serialization issues. The type field is unnecessary since type discrimination is already handled by the [JsonDerivedType] attribute with "channel" as the discriminator, and the TODO comment confirms this is a recognized issue.

    Medium
    • Update

    Co-authored-by: Michael Render <[email protected]>
    @nvborisenko nvborisenko merged commit e6f45aa into SeleniumHQ:trunk Mar 16, 2025
    8 of 10 checks passed
    @nvborisenko nvborisenko deleted the bidi-nested-types branch March 16, 2025 10:45
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    2 participants